Мой улов за неделю

[dskozin]

Вот вопрос, а что возвращает

List<UserEntity> getUsersForGroup(Long groupId) {
  if (groupId == null) {
    return Collections.emptyList();
  }

  return userRepository.findUsersByGroup(groupId);
}

когда ни одного пользователя не найдено?

[peresada]

какой-то странный вопрос… Очевидно, если в группе нет пользователей, то возвращает нулл\пустой список

[tsypanov]

В Spring Data запросы вида

@Query("select u from UserEntity u where u.group.id = :groupId")
List<UserEntity> findUsersByGroup(@Param("groupId") Long groupId);

возвращают пустой список (ArrayList) для пустой выборки.

[valery1707]

Так делает, на сколько я помню, не сам Spring Data, но и вообще любой ORM-маппинг, да и вообще чистый JDBC.

[tsypanov]

Про чистый JDBC не скажу, а Hibernate возвращает пустой список (ЕМНИП, пустой ArrayList).

[valery1707]

Да, я как раз про это: все возвращают пустую коллекцию если запрос вернул 0 записей.
В том смысле что ответ на исходный вопрос пустая коллекция, но не потому что так делает Spring Data, а потому что так делают все и всегда и нет причин по которым кто-то будет отдавать что-то другое кроме пустой коллекции.
Возможно dskozin предположил что UserRepository#findUsersByGroup может возвращать null по аналогии с методами возвращающими одиночный объект. Но тут как раз все нормально: если мы не можем найти объект, то представлением пустого результата будет как раз null за неимением лучшего варианта. А для коллекций пустой result set запроса это пустая коллекция.

Если перевести все запросы возвращающие одиночный объект на Optional<T> то там тоже можно избавиться от null.
Вот это как раз умеет Spring Data.

[dskozin]

Да. Как раз это я и имел ввиду, предполагая что ответ возможно необходимо обработать. И вообще как-то не совсем синхронное ощущение от этого метода, поскольку есть и прямой возврат пустой коллекции и возврат чего-то, что возвращает репозиторий, а его возврат в свою очередь зависит от магии и т.п. Т.е. получается, что если меняется тип возвращаемого значения, то его нужно менять в двух местах…

[tsypanov]

По поводу двойственного ощущения я ниже оставил комментарий про `SimpleJpaRepository::findAll`.

[valery1707]

Т.е. получается, что если меняется тип возвращаемого значения, то его нужно менять в двух местах…

Вы сейчас о чём?
Во всех вариантах метода getUsersForGroup тип (UserEntity) упоминается 1 раз — в джеренике на возвращаемом листе. Ну и ещё раз он, естественно присутствует в дженерике листа у UserRepository#findUsersByGroup.
Да, это два места. Но если мы меняем название класса сущности то, естественно, в обоих местах его и нужно будет сменить.

И я не понимаю как вопрос а что возвращает getUsersForGroup(Long) когда ни одного пользователя не найдено? превратился в если меняется тип возвращаемого значения, то его нужно менять в двух местах?

И вообще как-то не совсем синхронное ощущение от этого метода, поскольку есть и прямой возврат пустой коллекции и возврат чего-то, что возвращает репозиторий, а его возврат в свою очередь зависит от магии и т.п.

Где там магия?

• снаружи просят список пользователей для группы
• у нас (это проистекает из логики реализации) пользователь обязан иметь группу (то есть мы не интерпретируем группу null как пользователи без группы)
• Так что мы разделяем логику на два ветки, в зависимости от значения идентификатора группы:
  
  • null: пустая коллекция, так как мы сразу и заранее знаем что нет таких пользователей
  • не null: запрос в БД, который всегда возвращает коллекцию

В результате метод всегда возвращает коллекцию и никогда null.
По факту на метод можно (и нужно, раз уж они упоминаются в статье) добавить аннотацию @Nonnull.

Опять же на счёт магии запроса к БД: нет там магии.
Мы выполняем запрос в БД вида (упрощённо) SELECT * FROM user WHERE group_id = ? и этот запрос, при любом значении параметра, любоая БД обработает одинаково и без магии — вернёт коллекцию строк подходящих под наше условие. И да, в некоторых случаях эта коллекция может быть пустой и это нормально.

[PqDn]

Мне кажется такое надо решать на уровне код ревью и не выносить на общее обозрение. Или хотя бы статью обезличить.

[tsypanov]

Разумеется, это всё было найдено и исправлено именно в ходе код вычитки.

[ris58h]

Collection<Dto> dtos = getDtos();
Set<Integer> ids =  dtos.stream().map(Dto::getId).collect(toSet());
Stream.of(1,2,3,4,5)
  .filter(ids::contains)
  .collect(toList());

Я, может, чего не понял, но, вроде, задача — получить те id, что есть в списке (1,2,3,4,5). Зачем тогда собирать в Set id-шники из DTO (их же может быть 100500), когда можно взять Set.of(1,2,3,4,5) и бежать по списку DTO?

[bystr1k]

Мне кажется, автор имел в виду про то, что надо выносить инварианты из циклов. Это просто как показательный пример

[PqDn]

Имелось ввиду, что можно написать эффективней, например так

Set<Integer> search = Set.of(1,2,3,4,5);
dtos.stream()
    .map(Dto::getId)
    .filter(search::contains)
    .distinct()
    .collect(toList())

[tsypanov]

bystr1k прав, этот пример про инварианты. Далеко не все из них обнаруживаются статическими анализаторами.

[valery1707]

А вот ваш исходный

List<UserEntity> getUsersForGroup(Long groupId) {
  Optional<Long> optional = Optional.ofNullable(groupId);

  if (optional.isPresent()) {
    return userRepository.findUsersByGroup(optional.get());
  }

  return Collections.emptyList();   
}

я бы преобразовал в

List<UserEntity> getUsersForGroup(Long groupId) {
  return Optional
    .ofNullable(groupId)
    .map(userRepository::findUsersByGroup)
    .orElseGet(Collections::emptyList);
}

вместо вашего варианта:

List<UserEntity> getUsersForGroup(Long groupId) {
  if (groupId == null) {
    return Collections.emptyList();
  }

  return userRepository.findUsersByGroup(groupId);
}

[tsypanov]

Тоже вариант, только в этом случае orElseGet не даёт преимущества перед orElse, т. к. Collections.emptyList() возвращает кэшированный список.

[valery1707]

В случае с orElse метод Collections.emptyList() будет вызван в любом случае был ли у нас null на входе или нет, а в случае с orElseGet в него будет передан method reference который будет вызван только при необходимости.
Это алгоритмическая разница. Есть ли разница в производительности — я не измерял, и возможно orElse(Collections.emptyList()) будет даже эффективнее orElseGet(Collections::emptyList).
Но тут вопрос в том как будет в дальнейшем использоваться результат выполнения getUsersForGroup — ведь коллекцию, полученную из Collections.emptyList(), нельзя модифицировать. А если нам нужна такая возможность, то orElseGe(Collections::emptyList) можно заменить на orElseGet(ArrayList::new) и тут уже будет явный профит, по сравнению с orElse(new ArrayList())

[tsypanov]

По поводу дальнейшего использования, я тут посмотрел внутрь org.springframework.data.jpa.repository.support.SimpleJpaRepository, там есть такой метод

public List<T> findAll(Iterable<ID> ids) {

    if (ids == null || !ids.iterator().hasNext()) {
        return Collections.emptyList();
    }

    if (entityInformation.hasCompositeId()) {

        List<T> results = new ArrayList<T>();

        for (ID id : ids) {
            results.add(findOne(id));
        }

        return results;
    }

    ByIdsSpecification<T> specification = new ByIdsSpecification<T>(entityInformation);
    TypedQuery<T> query = getQuery(specification, (Sort) null);

    return query.setParameter(specification.parameter, ids).getResultList();
}

В текущем виде возможны два случая, когда возвращается пустой список: 1) на вход пришла пустая коллекция ключей и 2) запрос вернул пустую выборку.

Так вот в первом случае возвращается неизменяемый список, а во втором — ArrayList.

[valery1707]

Если точнее, то реализация SimpleJpaRepository#findAll(java.lang.Iterable<ID>) возвращает разные типы коллекций, в зависимости от значения аргумента и структуры сущности:

• null: java.util.Collections.EmptyList
• пустая коллекция: java.util.Collections.EmptyList
• сущность имеет композитный ключ: java.util.ArrayList из всех результатов SimpleJpaRepository#findOne(ID)
• сущность с некомпозитным ключом: то что вернёт ниже лежащий уровень, в случае с Hibernate это:
  
  • java.util.Collections.EmptyList: не совсем ясно когда это возможно, но в org.hibernate.internal.SessionImpl#list(java.lang.String, org.hibernate.engine.spi.QueryParameters) есть такой флоу — не уверен на сколько он реалистичен
  • java.util.ArrayList: если дело дошло до выполнения запроса

Это можно представить в виде двух ответов на вопрос нужно ли реально выполнять запрос на БД:

• нет: java.util.Collections.EmptyList и хватит с вас
• да: java.util.ArrayList

и опять же — никакой магии.

[valery1707]

А вот тут

try {
  FileChannel sc = new FileInputStream(src).getChannel();
  FileChannel dc = new FileOutputStream(new File(targetName)).getChannel();
  sc.transferTo(0, sc.size(), dc);
  dc.close();
  sc.close();
} catch (IOException ex) {
  log.error("ой", ex);
}

, кроме всего уже описанного, можно не закрыть один и даже оба канала, что чревато всякими бяками.
И, в случае отсутствия библиотечных функций, для таких вещей, как минимум, необходимо использовать try-with-resources. Благо он появился уже очень давно.

[valery1707]

А вот тут

static <T, Q extends Query> void setParams(
  Map<String, Collection<T>> paramMap,
  Set<String> notReplacedParams,
  Q query) {
    Set<String> params = paramMap.keySet();
    notReplacedParams.stream()
      .filter(params::contains)
      .forEach(param -> query.setParameter(param, paramMap.get(param)));
}

нет необходимости в отдельной переменной, так как вместо params::contains можно сделать сразу paramMap.keySet()::contains.

P.S.
А ещё этот тот самый void-метод меняющий состояние аргумента.