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

Чем больше разработчик работает над приложением в команде и чем лучше знает его код, тем чаще он занимается вычиткой творчества своих товарищей. Сегодня я покажу, что может быть выловлено за одну неделю в коде, написанном весьма неплохими разработчиками. Под катом собрание ярких артефактов нашего творчества (и немного моих размышлений).

Компараторы

Есть код:

@Getter
class Dto {
  private Long id;
}

// another class

List<Long> readSortedIds(List<Dto> list) {
  List<Long> ids = list.stream().map(Dto::getId).collect(Collectors.toList());
  ids.sort(new Comparator<Long>() {
    public int compare(Long o1, Long o2) {
      if (o1 < o2) return -1;
      if (o1 > o2) return 1;
      return 0;
    }
 });
 return ids;
}

Кто-то отметит, что сортировать можно напрямую стрим, я же хочу обратить ваше внимание на компаратор. В документации к методу Comparator::compare английским по белому написано:

Compares its two arguments for order. Returns a negative integer, zero, or a positive integer as the first argument is less than, equal to, or greater than the second.

Именно это поведение и реализовано в нашем коде. Что же не так? Дело в том, что создатели явы весьма дальновидно предположили, что подобный компаратор понадобится многим и сделали его за нас. Нам же остаётся им воспользоваться упростив наш код:

List<Long> readSortedIds(List<Dto> list) {
  List<Long> ids = list.stream().map(Dto::getId).collect(Collectors.toList());
  ids.sort(Comparator.naturalOrder());
  return ids;
}

Точно так же этот код

List<Dto> sortDtosById(List<Dto> list) {
  list.sort(new Comparator<Dto>() {
    public int compare(Dto o1, Dto o2) {
       if (o1.getId() < o2.getId()) return -1;
       if (o1.getId() > o2.getId()) return 1;
       return 0;
    }
  });
  return list;
}

лёгким движением руки превращается в такой

List<Dto> sortDtosById(List<Dto> list) {
  list.sort(Comparator.comparing(Dto::getId));
  return list;
}

Кстати, в новой версии "Идеи" можно будет делать вот так:

Окатиться чародейством

Злоупотребление Optional-ами

Наверное, каждый из нас видел что-то вроде этого:

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

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

  return Collections.emptyList();   
}

Часто Optional используют для проверки наличия/отсутствия значения, хотя создан он был не для этого. Завязывайте со злоупотреблением и пишите проще:

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

  return userRepository.findUsersByGroup(groupId);
}

Помните, что Optional — это не про аргумент метода или поле, а про возвращаемое значение. Именно поэтому он спроектирован без поддержки сериализации.

void-методы, меняющие состояние аргумента

Представьте такой метод:

@Component
@RequiredArgsConstructor
public class ContractUpdater {
  private final ContractRepository repository;

  @Transactional
  public void updateContractById(Long contractId, Dto contractDto) {
    Contract contract = repository.findOne(contractId);
    contract.setValue(contractDto.getValue());
    repository.save(contract);
  }
}

Наверняка вы много раз видели и писали что-то подобное. Здесь мне не нравится, что метод меняет состояние сущности, но не возвращает её. Как ведут себя подобный методы фреймворков? Например, org.springframework.data.jpa.repository.JpaRepository::saveи javax.persistence.EntityManager::merge возвращают значение. Предположим, после обновления контракта нам нужно получить его за пределами метода update. Получается что-то вроде этого:

@Transactional
public void anotherMethod(Long contractId, Dto contractDto) {
  updateService.updateContractById(contractId, contractDto);
  Contract contract = repositoroty.findOne(contractId);
  doSmth(contract);
}

Да, мы могли бы передавать сущность напрямую в метод UpdateService::updateContract, изменив его сигнатуру, но лучше сделать вот так:

@Component
@RequiredArgsConstructor
public class ContractUpdater {
  private final ContractRepository repository;

  @Transactional
  public Contract updateContractById(Long contractId, Dto contractDto) {
    Contract contract = repository.findOne(contractId);
    contract.setValue(contractDto.getValue());
    return repository.save(contract);
  }
}

//использование

@Transactional
public void anotherMethod(Long contractId, Dto contractDto) {
  Contract contract = updateService.updateContractById(contractId, contractDto);
  doSmth(contract);
}

С одной стороны, это помогает упростить код, с другой — помогает при тестировании. Вообще тестирование void методов на редкость муторная задача, что я покажу, используя этот же пример:

@RunWith(MockitoJUnitRunner.class)
public class ContractUpdaterTest {
  @Mock 
  private ContractRepository repository;
  @InjectMocks
  private ContractUpdater updater;

  @Test
  public void updateContractById() {
    Dto dto = new Dto();
    dto.setValue("что-то там");

    Long contractId = 1L;
    when(repository.findOne(contractId)).thenReturn(new Contract());

    updater.updateContractById(contractId, contractDto); //void

    //как проверить, что в контракт вписали значение из dto? - Примерно так:
    ArgumentCaptor<Contract> captor = ArgumentCaptor.forClass(Contract.class);
    verify(repository).save(captor.capture());
    Contract updated = captor.getValue();

    assertEquals(dto.getValue(), updated.getValue());
  }
}

А ведь всё можно сделать проще, если метод возвращает значение:

Убедиться

@RunWith(MockitoJUnitRunner.class)
public class ContractUpdaterTest {
  @Mock 
  private ContractRepository repository;
  @InjectMocks
  private ContractUpdater updater;

  @Test
  public void updateContractById() {
    Dto dto = new Dto();
    dto.setValue("что-то там");

    Long contractId = 1L;
    when(repository.findOne(contractId)).thenReturn(new Contract());

    Contract updated = updater.updateContractById(contractId, contractDto);

    assertEquals(dto.getValue(), updated.getValue());
  }
}

Одним махом проверяется не только вызов ContractRepository::save, но и правильность сохранённого значения.

Велосипедостроение

Прикола ради, откройте свой проект и поищите вот это:

lastIndexOf('.')

С большой вероятностью всё выражение выглядит примерно так:

String fileExtension = fileName.subString(fileName.lastIndexOf('.'));

Вот о чём ни один статический анализатор не может предупредить, так это о вновь изобретённом велосипеде. Господа, если вы решаете некую задачу, связанную с именем/расширением файла или путём к нему, ровно как и с чтением/записью/копирование, то в 9 случаях из 10 задача уже решена до вас. Поэтому завязывайте с велосипедостроением и берите готовые (и проверенные) решения:

import org.apache.commons.io.FilenameUtils;

//...
String fileExtension = FilenameUtils.getExtension(fileName);

В этом случае вы сберегает время, которое было бы потрачено на проверку годности велосипеда, а также получаете более продвинутый функционал (см. документацию FilenameUtils::getExtension).

Или вот такой код, копирующий содержимое одного файла в другой:

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);
}

Какие обстоятельства могут нам помешать? Тысячи их:

• точка назначения может оказаться папкой, а вовсе не файлом
• источник может оказаться папкой
• источник и точка назначения могут оказаться одним и тем же файлом
• точку назначения невозможно создать
• и т. д. и т. п.

Печаль в том, что пользуясь самописью об этом всём мы узнаем уже в ходе копирования.
Если же сделаем по уму

import org.apache.commons.io.FileUtils;

try {
  FileUtils.copyFile(src, new File(targetName));
} catch (IOException ex) {
  log.error("ой", ex);
}

то часть проверок будет выполнена ещё до начала копирования, а возможное исключение будет более информативным (см. исходники FileUtils::copyFile).

Пренебрежение @Nullable/@NotNull

Предположим, у нас есть сущность:

@Entity 
@Getter
public class UserEntity {
  @Id
  private Long id;

  @Column
  private String email;

  @Column
  private String petName;
}

В нашем случае колонка email в таблице описана как not null, в отличие от petName. Т. е. мы можем пометить поля соответствующими аннотациями:

import javax.annotation.Nullable;
import javax.annotation.NotNull;

//...

  @Column
  @NotNull
  private String email;

  @Column
  @Nullable
  private String petName;

На первый взгляд, это выглядит как подсказка разработчику, и это действительно так. Вместе с тем эти аннотации — куда более могущественное средство, чем обычная метка.

Например, их понимают среды разработки, и если после добавления аннотаций мы попробуем сделать вот так:

boolean checkIfPetBelongsToUser(UserEnity user, String lostPetName) {
  return user.getPetName().equals(lostPetName);
}

то "Идея" предупредит нас об опасности таким сообщением:

Method invocation 'equals' may produce 'NullPointerException'

В коде

boolean hasEmail(UserEnity user) {
  boolean hasEmail = user.getEmail() == null;
  return hasEmail;
}

будет другое предупреждение:

Condition 'user.getEmail() == null' is always 'false'

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

Если мои доводы кажутся неубедительными, то смотрите исходники любого серьёзного проекта, того же "Спринга" — они увешаны аннотациями как новогодняя ёлка. И это не блажь, а суровая необходимость.

Единственный недостаток, как мне кажется, — необходимость постоянно поддерживать аннотации в современном состоянии. Хотя, если разобраться, — это скорее благо, ведь возвращаясь раз за разом к коду мы понимаем его всё лучше.

Невнимательность

В этом коде нет ошибок, но есть излишество:

Collection<Dto> dtos = getDtos();
Stream.of(1,2,3,4,5)
  .filter(id -> {
    List<Integer> ids =  dtos.stream().map(Dto::getId).collect(toList());
    return ids.contains(id);
  })
  .collect(toList());

Непонятно, зачем создавать новый список ключей, по которому выполняется поиск, если он не меняется при проходе по стриму. Хорошо, что элементов всего 5, а если их будет 100500? А если метод getDtos вернёт 100500 объектов (в списке!), то какая производительность будет у этого кода? Никакая, поэтому лучше вот так:

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());

Также и здесь:

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

Очевидно, что значение, возвращаемое выражение inParameterMap.keySet() неизменно, поэтому его можно вынести в переменную:

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)));
}

Кстати, такие участки можно вычислить используя проверку 'Object allocation in a loop'.

Когда статический анализ бессилен

Восьмая ява давно отгремела, но все мы любим стримы. Некоторые из нас любят их так сильно, что используют повсюду:

public Optional<EmailAdresses> getUserEmails() {
  Stream<UserEntity> users = getUsers().stream();

  if (users.count() == 0) {
    return Optional.empty();
  }

  return users.findAny();
}

Стрим, как известно, свеж до вызова на нём завершения, поэтому повторное обращение к переменной users в нашем коде приведёт к IllegalStateException.

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

Мне кажется, что использование переменных типа Stream, равно как их передача в качестве аргументов и возврат из методов, похоже на хождение по минному полю. Может повезти, а может и нет. Отсюда простое правило: любое появление Stream<T> в коде должно проверяться (а по-хорошему сразу же выпиливаться).

Простые типы

Многие уверены, что boolean, int и т. п. — это только про производительность. Отчасти это так, но кроме того простой тип not null по умолчанию. Если целочисленное поле сущности ссылается на колонку, которая объявлена в таблице как not null, то имеет смысл использовать int, а не Integer. Это своего рода комбо — и потребление памяти ниже, и код упрощается ввиду ненужности проверок на null.

На этом всё. Помните, что всё вышеперечисленное не является истиной в последней инстанции, думайте своей головой и осмысленно подходите к применению любых советов.