Чем больше разработчик работает над приложением в команде и чем лучше знает его код, тем чаще он занимается вычиткой творчества своих товарищей. Сегодня я покажу, что может быть выловлено за одну неделю в коде, написанном весьма неплохими разработчиками. Под катом собрание ярких артефактов нашего творчества (и немного моих размышлений).
Компараторы
Есть код
@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.
На этом всё. Помните, что всё вышеперечисленное не является истиной в последней инстанции, думайте своей головой и осмысленно подходите к применению любых советов.
ссылка на оригинал статьи https://habr.com/post/426105/
Добавить комментарий