3

Скажите, надо и если надо то как можно оптимизировать этот код? возможно заменить это джойнами из базы, а не этими двух этажными циклами или что лучше почитать об оптимизации в JAVA, чтобы перестать писать этот га*нокод, как говорят мои коллеги. Так как на этом этапе, я Trainee/Junior но очень бы хотел улучшить свой скилл в программирование и начать писать хороший код.

public List<Place> getPlacesByPlaceTypeId(PlaceTypeForm placeType) {

    List<Place> allPlaces = getAllPlaces();
    List<Place> places = new ArrayList<Place>();
    for (Place place : allPlaces) {
        if (place.getId() == placeType.getId()) {
            places.add(place);
        }
    }

    return places;
}

public void addPlace(PlaceForm placeForm, MultipartFile multipartFile){

    Place place = new Place();
    PlaceType placeType = placeTypeDao.getPlaceTypeById(placeForm.getPlaceTypeId());
    place.setName(placeForm.getName());
    place.setFile(fileService.saveFile(multipartFile, "place"));
    place.getPlaceTypes().add(placeType);
    placeType.getPlaces().add(place);
}

public void deletePlace(PlaceForm placeForm){
    Place place = placeDao.getPlaceById(placeForm.getId());
    List<PlaceType> placeTypes = new ArrayList<PlaceType>(place.getPlaceTypes());
    List<Menu> menus = new ArrayList<Menu>(place.getMenus());
    Iterator<PlaceType> placeTypeList = placeTypes.iterator();
    while(placeTypeList.hasNext()){
        placeTypeList.next().getPlaces().remove(place);
    }
    place.getPlaceTypes().removeAll(placeTypes);
    place.getMenus().removeAll(menus);
    placeDao.deletePlace(place);
}

public void deletePlaceFromPlaceType(PlaceForm placeForm){
    PlaceType placeType = placeTypeDao.getPlaceTypeById(placeForm.getPlaceTypeId());
    placeType.getPlaces().remove(placeDao.getPlaceById(placeForm.getId()));
}

public boolean addPlaceToAnyPlaceType(PlaceForm placeForm){
    PlaceType placeType = placeTypeDao.getPlaceTypeById(placeForm.getPlaceTypeId());
    List<Place> places = new ArrayList<Place>(placeType.getPlaces());
    Iterator<Place> placeList = places.iterator();
    while(placeList.hasNext()){
        if(placeList.next().equals(placeDao.getPlaceById(placeForm.getId()))){
            return false;
        }
    }
    placeType.getPlaces().add(placeDao.getPlaceById(placeForm.getId()));
    return true;
}
4
  • 2
    Так если вы Trainee/Junior, почему бы вашим коллегам вам не помочь? PS. я видимо тоже говнокодер, потому что я ничего принципиально плохого не вижу в вашем коде.
    – Vartlok
    18 янв 2016 в 10:08
  • во во, согласен с @Vartlok, да и что Вы понимаете под оптимизацией? оптимизацию производительности, или же просто сделать код более красивым)
    – ermak0ff
    18 янв 2016 в 10:12
  • 1
    @ermak0ff оптимизацию производительности. Нужно сделать рефактор этой части кода, чтобы ускорить процесс загрузки. Я так себе подумал, что это скорее всего можно было бы сделать join'ами напрямую из базы, вместо того, чтобы сравнивать айдишки и потом сортировать в зависимости от результата, но существенно ли это что-то изменит? А на счет красоты, я свой код в любом виде люблю, он для меня всегда красивый )
    – calm27
    18 янв 2016 в 10:20
  • Мне одному этот код напомнил про Execution in the Kingdom of Nouns?
    – VladD
    18 янв 2016 в 11:31

2 ответа 2

7

Вы хотите провести рефакторинг кода, а не оптимизацию. В приведенном участке кода оптимизировать нечего, либо это слишком не существенно.

  1. Если это класс с набором методов для работы с объектами класса Place, то нет необходимости в названии каждого метода об этом упоминать.

  2. Также заменим delete на remove (стандартное имя метода для удаления элемента из коллекции в Java).

  3. Методу getPlacesByPlaceTypeId(PlaceTypeForm placeType) требуется только идентификатор. Зачем передавать весь объект? То же самое касается метода deletePlace(PlaceTypeForm placeType) и метода deletePlaceFromPlaceType(PlaceTypeForm placeType).

  4. Старайтесь давать более осмысленные имена переменным/методам/классам. Например, если MultipartFile multipartFile в методе addPlace - это присоединяемый файл к месту с его описанием, то можно назвать аргумент, как descriptionFile или attachedFile. Тут уже зависит от контекста, которого я не знаю.

Обновленный интерфейс класса:

public List<Place> getByTypeId(int typeId);
public void add(PlaceForm form, MultipartFile attachedFile);
public void remove(int formId);
  1. Почему у вас PlaceType (тип места) должен хранить список мест? Это совершенно не его обязанность! Необходимо пересмотреть архитектуру. Тип места - это свойство объекта класса Place, которое может принимать значения из допустимых PlaceType (если это перечисление). Соответственно получение списка мест по определенному типу - это правильно сформированный запрос к БД и все.

  2. Зачем в методе getPlacesByPlaceTypeId() получать все места, а затем их фильтровать? А если объектов будет несколько десятков тысяч? Напишите в вашем DAO нормальный запрос, который сразу будет возвращать все объекты с определенным идентификатором.

  3. Цепочки такого рода placeType.getPlaces().add(placeDao.getPlaceById(placeForm.getId())); трудно читать и отлаживать (почитайте, например, enSO, где обсуждается этот вопрос).

Можно продолжать и дальше, но думаю и этого достаточно.

а не этими двух этажными циклами или что лучше почитать об оптимизации в JAVA, чтобы перестать писать этот га*нокод, как говорят мои коллеги.

Не знаю при чем здесь оптимизация, либо вы не правильно поняли слова коллег. Прочитайте про принципы проектирования, почитайте "Совершенный код" Стива Макконелла.

1
  • Почему у вас PlaceType (тип места) должен хранить список мест? так как здесь связь ManyToMany, я посчитал, что это было бы правильным оформлением, но я учту и пересмотрю архитектуру.
    – calm27
    18 янв 2016 в 12:16
3

Такой код неудобно поддерживать/отлаживать.

Возьмем данную строчку кода:

public void deletePlaceFromPlaceType(PlaceForm placeForm){
    PlaceType placeType = placeTypeDao.getPlaceTypeById(placeForm.getPlaceTypeId());
    placeType.getPlaces().remove(placeDao.getPlaceById(placeForm.getId()));
}

Тут: placeForm.getPlaceTypeId() вы можете словить NPE, если каким то образом на входе функции будет null, по этому имеет смысл добавить проверку, и если null это не нормальное поведение, то отреагировать соответствующе, например кинуть исключение с нормальным описанием проблемы.

Ваши DAO вряд ли могут быть равными null, по этому чекать их нет смысла.

Реультат вызова placeForm.getPlaceTypeId() лучше вынести в отдельную переменную. Во время отладки удобнее будет посмотреть значение переменной, до того, как оно попадет в DAO. Не бойтесь заводить новые переменные, компилятор умеет оптимизировать подобное.

placeType.getPlaces() так же стоит вынести в отдельную переменную, все так же ради удобной отладки. Если getPlaces() может вернуть null, то необходимо добавить проверку.

Ну и так далее со всем кодом. Сейчас это просто стенка непонятного и не читабельного кода. Правильный же код должен читаться как текст.

Что касается оптимизации, то займитесь написанием более оптимальных обращений к базе, пользуйтесь индексами. Особо требовательные запросы пишите на SQL'e.

4
  • разве это не сделает код еще более нагруженным и стенку еще более громоздкой?
    – calm27
    18 янв 2016 в 10:24
  • @Khodan.D. нет, современные компиляторы очень продвинулись в вопросах оптимизации. Визуально кода станет больше, но его будет гораздо проще отлаживать. Сейчас же ваш код отлаживать будет не удобно. 18 янв 2016 в 10:31
  • а насколько правильным будет заменить циклы джойнами из базы ? Это сделает код проще и лучше или ничего не изменит?
    – calm27
    18 янв 2016 в 10:35
  • 1
    @Khodan.D. сделает это проще запрос или нет, вы можете узнать построив план запроса 18 янв 2016 в 12:19

Ваш ответ

By clicking “Отправить ответ”, you agree to our terms of service and acknowledge you have read our privacy policy.

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.