Оптимален ли код ниже для данной задачи?

Question

[Валентин]

Доброго дня всем! Сейчас переделываю архитектуру приложения, точнее его категории. Внедрил Materialized Path+ Adjacency List. И при добавлении новой категории, необходимо добавить ей полный путь. Можно ли sql запросом вытащить значения после последней точки у необходимой группы категории? Т.е есть такое значение в таблице: PATH: 1123.3131.33 Мне нужно получить 33, т.е то, что после последней запятой. И эту операцию необходимо провести у всей подгруппы, и найти наибольшее.В коде ниже я вытаскиваю ту группу категорий, которая мне необходима, и обрабатываю их. Оптимален ли этот код? Прошу оценить способ выборки.

public ResponseMessage addCategory(List<Category> newCategoryList, int parentId) {


        if (parentId == 0) {
            List<Category> categoryList = entityManager.createQuery("FROM Category c Where c.parentCategory.id=null", Category.class).getResultList();
            Integer largestIndex = getLargestIndex(categoryList);

            for (Category category : newCategoryList) {
                largestIndex++;
                String newPath = largestIndex.toString();
                category.setPath(newPath);
                entityManager.persist(category);

            }
            return new ResponseMessage(true, " ");

        } 

            Category parentCategory = entityManager.find(Category.class,  parentId);
            List<Category> categoryList = parentCategory.getCategoryList();
            categoryList.size();

            int largestIndex = getLargestIndex(categoryList);

            for (Category category : newCategoryList) {
                largestIndex++;
                StringBuilder newPath = new StringBuilder(parentCategory.getPath());
                newPath.append('.');
                newPath.append(largestIndex);
                parentCategory.addCategory(new Category(category.getName(), newPath));
            }
            return new ResponseMessage(true, " ");
   }

    public int getLargestIndex(List<Category> categoryList) {
        int largestIndex = 0;
        for (Category c : categoryList) {
            String[] sequence = c.getPath().split("\\.");
            int number = Integer.parseInt(sequence[sequence.length - 1]);
            if (number > largestIndex)
                largestIndex = number;
        }
        return largestIndex;
    }

p.s И оптимально ли разделение условием (parentId==0)? Т.к я решил не делать единого предка, а таким образом выбирать вышестоящие по иерархии.

Answer 1

[vayho]

Category parentCategory = entityManager.find(Category.class, 1);

Здесь вероятно вместо 1 нужно поставить parentId.

И ответ на ваш вопрос: вместо ручного инкремента largestIndex вам нужно использовать id последней добавленной записи.

Кстати раз уж вы используете hibernate то посмотрите в сторону Closure Table, этот паттерн как мне кажется более органично смотрится с hibernate.

Вот пример кода с id(обязательно обернуть в транзакцию):

public ResponseMessage addCategory(List<Category> newCategoryList, int parentId) {
    Category parentCategory = parentId == 0 ? null : entityManager.find(Category.class, parentId);

    for (Category category : newCategoryList) {
        entityManager.persist(category);
        entityManager.flush();
        if (parentCategory != null) {
            category.setPath(parentCategory.getPath() + "." + category.getId());
            category.setParent(parentCategory);
        } else {
            category.setPath(String.valueOf(category.getId()));
        }
    }
    return new ResponseMessage(true, " ");
}

Comments

[Валентин]

Вы правы, там должен быть Parent id , тесты не хотел писать, так и добавил значение, и сам код в спешке написан. Я тоже думал инкрементировать по последней добавленной записи, только для этого придется ввести дополнительно поле last или дату поставить, либо есть другой способ?

Если у вас будет возможность, то аргументируйте,пожалуйста, почему же код ужасен, для меня это очень важно,хоть я и писал в спешке, для себя я заметил косяк с id и лишний else.

[vayho]

Выше я привел пример кода(может не корректно с первого раза работать но суть примерно такая). Там выполнится два запроса один на вставку, второй на update, поэтому обязательно нужно выполнять запросы в транзакции.

Код ужасен потому что сложно читать и мелкие ошибки, например
ошибка с parentId,
ResponseMessage в DAO методе,
название метода addCategory когда передается список в качестве параметра,
parentId можно было бы найти и выше по коду, а сюда передать уже найденный объект, либо вообще просетить его в объекты в списке и не передавать второй параметр
int в качестве id
код не отформатирован(пустые строки и отступы)

[vayho]

Да, еще StringBuilder не используйте если элементы не добавляются в цикле. Конкатенацию компилятор Java оптимизирует сам.

[Валентин]

vayho, большое вам спасибо за пример! DAO слой обернут в прокси объект на уровне сервиса, так что во flush нет необходимости. Устанавливаю предка методом:
public void addCategory(Category category) {
category.setParentCategory(this);
categoryList.add(category);
} . А на счет ID, то на сервер приходят только имена категорий, как же у них могут быть ID, если они новые?)

[vayho]

Метод flush как раз поможет сгенерировать id. Persist лишь добавляет объект в контекст EntityManager, flush же выполняет непосредственно запрос на вставку не закрывая транзакцию. Соответственно после того как выполнится flush в объекте category появится id(конечно это работает если поле автоинкрементится).

[Валентин]

Большое вам спасибо, Vayho! Действительно, я не знаю зачем хотел, чтобы все новые PATH потомков, начинались с =)) Если было бы лучше и проще взять просто их ID. Очень интересный ход подсказали с flush . Но все равно я не знаю, что правильнее возвращать, в случае успеха или неудачи. Пока что я решил просто boolean значение вернуть. Но если у вас будет возможность, то могли бы вы подсказать, как обычно возвращают результат успеха или провала транзакции, в try/catch оборачивают и какую-нибудь exeption, связанный с ошибкой транзакции? Вот, кстати, код который получился, надеюсь все отступы учел:
public boolean addCategoryList(List newCategoryList, int parentId) {
boolean resultOfOperation=false;

Category parentCategory = parentId == 0 ? null : entityManager.find(Category.class, parentId);

for (Category category : newCategoryList) {
entityManager.persist(category);
entityManager.flush();

if (parentCategory != null) {
category.setParentCategory(parentCategory);
category.setPath(parentCategory.getPath() + "." + category.getId());
resultOfOperation=true;
} else{
category.setPath(String.valueOf(category.getId()));
resultOfOperation=true;
}
}
return resultOfOperation;
}

P.s только почему-то у меня все без flush генерится))

[vayho]

Возвращать ничего не нужно. Обычно где то сверху есть метод помеченный '@Transactional', он автоматически за вас сделает rollback в случае возникновения ошибки. Либо используйте где нибудь выше метода используйте EntityManager.getTransaction().begin/commit/rollback в try/catch.
Persist генерит id потому что вне транзакции(т.е. все операции сразу выполняются в БД). Это хреново, потому что весь процесс работы с БД не атомарен, а значит персистентность данных может быть нарушена(например тут может возникнуть ситуация когда не будет заполнен path).

Для форматирования кода комбинация в Eclipse ctrl + shift + f, в Intellij Idea ctrl + alt + l.

[Валентин]

Очень и очень странно, что процессы проходят вне транзакции т.к на уровне сервиса у меня есть :
@Service
@Transactional(propagation = Propagation.REQUIRED)
И вроде все должно четко работать, т.к я делаю вызовы через сервис. А как тогда клиенту вернуть сообщение или статус, что операция прошла успешно?Ведь если будет rollback, то он откатит только транзакцию. И код я почти все время форматирую в IDEA, правда, двойные отступы он у меня не убирает.

Answer 2

[Александр N++]

Кстати у вас } else {
лишний в первой функции достаточно условия. if( ... ) {
return
}
... другой код..

Можно ли sql запросом вытащить значения после последней точки у необходимой группы категории? Т.е есть такое значение в таблице: PATH: 1123.3131.33 Мне нужно получить 33, т.е то, что после последней запятой

select * from category where mpath like '%.33'

Comments

[Валентин]

Спасибо, не заметил на счет else-код в спешке писал. , но так я выберу конкретное значение после последней точки, а мне нужно вычленить значение после последней точки, не зная его