Оптимизация в JAVA

#java #оптимизация

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

public List getPlacesByPlaceTypeId(PlaceTypeForm placeType) {

    List allPlaces = getAllPlaces();
    List places = new ArrayList();
    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 placeTypes = new ArrayList(place.getPlaceTypes());
    List menus = new ArrayList(place.getMenus());
    Iterator 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 places = new ArrayList(placeType.getPlaces());
    Iterator 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;
}

    

Ответы

Ответ 1

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


Если это класс с набором методов для работы с объектами класса Place, то нет необходимости
в названии каждого метода об этом упоминать. 
Также заменим delete на remove (стандартное имя метода для удаления элемента из коллекции
в Java).
Методу getPlacesByPlaceTypeId(PlaceTypeForm placeType) требуется только идентификатор.
Зачем передавать весь объект? То же самое касается метода deletePlace(PlaceTypeForm
placeType) и метода deletePlaceFromPlaceType(PlaceTypeForm placeType).
Старайтесь давать более осмысленные имена переменным/методам/классам. Например, если
MultipartFile multipartFile в методе addPlace - это присоединяемый файл к месту с его
описанием, то можно назвать аргумент, как descriptionFile или attachedFile. Тут уже
зависит от контекста, которого я не знаю.


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

public List getByTypeId(int typeId);
public void add(PlaceForm form, MultipartFile attachedFile);
public void remove(int formId);



Почему у вас PlaceType (тип места) должен хранить список мест? Это совершенно не
его обязанность! Необходимо пересмотреть архитектуру. Тип места - это свойство объекта
класса Place, которое может принимать значения из допустимых PlaceType (если это перечисление).
Соответственно получение списка мест по определенному типу - это правильно сформированный
запрос к БД и все.
Зачем в методе getPlacesByPlaceTypeId() получать все места, а затем их фильтровать?
А если объектов будет несколько десятков тысяч? Напишите в вашем DAO нормальный запрос,
который сразу будет возвращать все объекты с определенным идентификатором.
Цепочки такого рода placeType.getPlaces().add(placeDao.getPlaceById(placeForm.getId()));
трудно читать и отлаживать (почитайте, например, enSO, где обсуждается этот вопрос).


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


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


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


Ответ 2

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

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

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.