Plus un développeur travaille sur une application en équipe et mieux connaît son code, plus il est souvent engagé dans la relecture du travail de ses camarades. Aujourd'hui, je vais montrer ce qui peut être capturé en une semaine dans du code écrit par de très bons développeurs. Sous la coupe se trouve une collection d'artefacts vifs de notre créativité (et un peu de mes pensées).
Comparateurs
Il y a un code:
@Getter class Dto { private Long id; }
Quelqu'un note que vous pouvez trier le flux directement, mais je veux attirer votre attention sur le comparateur. La documentation de la méthode Comparator::compare indique en anglais en blanc:
Compare ses deux arguments en faveur de l'ordre. Renvoie un entier négatif, zéro ou un entier positif car le premier argument est inférieur, égal ou supérieur au second.
C'est ce comportement qui est implémenté dans notre code. Qu'est-ce qui ne va pas? Le fait est que les créateurs de Java ont laissé entendre à beaucoup de clairvoyance que beaucoup auraient besoin d'un tel comparateur et l'ont fait pour nous. Nous pouvons l'utiliser en simplifiant notre code:
List<Long> readSortedIds(List<Dto> list) { List<Long> ids = list.stream().map(Dto::getId).collect(Collectors.toList()); ids.sort(Comparator.naturalOrder()); return ids; }
De même, ce code
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; }
avec un coup de poignet se transforme en
List<Dto> sortDtosById(List<Dto> list) { list.sort(Comparator.comparing(Dto::getId)); return list; }
Soit dit en passant, dans la nouvelle version de «Idées», il sera possible de faire comme ceci:
Abus facultatif
Chacun de nous a probablement vu quelque chose comme ça:
List<UserEntity> getUsersForGroup(Long groupId) { Optional<Long> optional = Optional.ofNullable(groupId); if (optional.isPresent()) { return userRepository.findUsersByGroup(optional.get()); } return Collections.emptyList(); }
Souvent Optional utilisé pour vérifier la présence / absence d'une valeur, bien qu'il n'ait pas été créé pour cela. Attachez les abus et écrivez plus facilement:
List<UserEntity> getUsersForGroup(Long groupId) { if (groupId == null) { return Collections.emptyList(); } return userRepository.findUsersByGroup(groupId); }
N'oubliez pas que Optional ne concerne pas l'argument ou le champ de méthode, mais la valeur de retour. C'est pourquoi il est conçu sans prise en charge de la sérialisation.
vider les méthodes qui modifient l'état d'un argument
Imaginez une méthode comme celle-ci:
@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); } }
Vous avez sûrement vu et écrit quelque chose de similaire plusieurs fois. Ici, je n'aime pas que la méthode change l'état de l'entité, mais ne le renvoie pas. Comment se comportent les méthodes-cadres similaires? Par exemple, org.springframework.data.jpa.repository.JpaRepository::save et javax.persistence.EntityManager::merge renvoient une valeur. Supposons qu'après la mise à jour d'un contrat, nous devons l'extraire de la méthode de update . Il s'avère quelque chose comme ceci:
@Transactional public void anotherMethod(Long contractId, Dto contractDto) { updateService.updateContractById(contractId, contractDto); Contract contract = repositoroty.findOne(contractId); doSmth(contract); }
Oui, nous pourrions passer l'entité directement à la méthode UpdateService::updateContract en modifiant sa signature, mais il vaut mieux le faire comme ceci:
@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); } }
D'une part, cela aide à simplifier le code, d'autre part, cela aide aux tests. En général, tester des méthodes void est une tâche extrêmement morne, que je montrerai en utilisant le même exemple:
@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);
Mais tout peut être simplifié si la méthode renvoie une valeur:
Assurez-vous @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()); } }
D'un seul coup, non seulement l'appel à ContractRepository::save vérifié, mais aussi l'exactitude de la valeur enregistrée.
Construction de vélos
Pour le plaisir, ouvrez votre projet et recherchez ceci:
lastIndexOf('.')
Avec une probabilité élevée, l'expression entière ressemble à ceci:
String fileExtension = fileName.subString(fileName.lastIndexOf('.'));
C'est ce qu'aucun analyseur statique ne peut avertir, il s'agit du vélo nouvellement inventé. Messieurs, si vous résolvez un certain problème lié au nom / extension de fichier ou au chemin d'accès, tout comme la lecture / l'écriture / la copie, dans 9 cas sur 10, la tâche a déjà été résolue devant vous. Par conséquent, associez-vous à la construction de vélos et prenez des solutions toutes faites (et éprouvées):
import org.apache.commons.io.FilenameUtils;
Dans ce cas, vous gagnez du temps qui serait consacré à la vérification de l'adéquation du vélo, et bénéficiez également de fonctionnalités plus avancées (voir la documentation FilenameUtils::getExtension ).
Ou voici un code qui copie le contenu d'un fichier dans un autre:
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); }
Quelles circonstances peuvent nous empêcher? Des milliers d'entre eux:
- la destination peut être un dossier, pas un fichier du tout
- la source peut être un dossier
- la source et la destination peuvent être le même fichier
- la destination ne peut pas être créée
- etc. et ainsi de suite.
Ce qui est triste, c'est qu'en utilisant l'auto-enregistrement sur tout cela, nous apprenons déjà pendant la copie.
Si nous le faisons judicieusement
import org.apache.commons.io.FileUtils; try { FileUtils.copyFile(src, new File(targetName)); } catch (IOException ex) { log.error("", ex); }
alors une partie des vérifications sera effectuée avant le début de la copie, et une éventuelle exception sera plus informative (voir les sources de FileUtils::copyFile ).
Négliger @ Nullable / @ NotNull
Supposons que nous ayons une entité:
@Entity @Getter public class UserEntity { @Id private Long id; @Column private String email; @Column private String petName; }
Dans notre cas, la colonne email - email du tableau est décrite comme not null , contrairement à petName. Autrement dit, nous pouvons marquer les champs avec les annotations correspondantes:
import javax.annotation.Nullable; import javax.annotation.NotNull;
À première vue, cela ressemble à un indice pour le développeur, et c'est vraiment le cas. En même temps, ces annotations sont un outil beaucoup plus puissant qu'une étiquette régulière.
Par exemple, les environnements de développement les comprennent et si, après avoir ajouté des annotations, nous essayons de le faire comme ceci:
boolean checkIfPetBelongsToUser(UserEnity user, String lostPetName) { return user.getPetName().equals(lostPetName); }
alors "l'Idée" nous avertira du danger avec ce message:
L'appel de méthode «égal» peut produire «NullPointerException»
Dans le code
boolean hasEmail(UserEnity user) { boolean hasEmail = user.getEmail() == null; return hasEmail; }
il y aura un autre avertissement:
La condition 'user.getEmail () == null' est toujours 'false'
Cela aide la censure intégrée à trouver les erreurs possibles et nous aide à mieux comprendre l'exécution du code. Dans le même but, les annotations sont utiles à placer sur les méthodes qui renvoient une valeur et leurs arguments.
Si mes arguments ne semblent pas concluants, alors regardez le code source de tout projet sérieux, le même "Spring" - ils sont accrochés avec des annotations comme un arbre de Noël. Et ce n'est pas un caprice, mais une grave nécessité.
Le seul inconvénient, me semble-t-il, est la nécessité de maintenir constamment les annotations dans un état moderne. Bien que, si vous le regardez, c'est plutôt une bénédiction, car en revenant au code encore et encore, nous le comprenons de mieux en mieux.
Insouciance
Il n'y a pas d'erreur dans ce code, mais il y a un excès:
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());
Il n'est pas clair pourquoi créer une nouvelle liste de clés par lesquelles la recherche est effectuée, si elle ne change pas lors du passage dans le flux. C'est bien qu'il n'y ait que 5 éléments, mais que se passe-t-il s'il y en a 100500? Et si la méthode getDtos renvoie 100500 objets (dans la liste!), Alors quel genre de performances ce code aura-t-il? Aucun, donc c'est mieux comme ça:
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());
Ici aussi:
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))); }
De toute évidence, la valeur renvoyée par l'expression inParameterMap.keySet() est inchangée, elle peut donc être déplacée vers une variable:
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))); }
Soit dit en passant, ces sections peuvent être calculées à l'aide de la vérification «Allocation d'objet dans une boucle».
Quand l'analyse statique est impuissante
Huitième Java s'est éteint depuis longtemps, mais nous aimons tous les streams. Certains d'entre nous les aiment tellement que nous les utilisons partout:
public Optional<EmailAdresses> getUserEmails() { Stream<UserEntity> users = getUsers().stream(); if (users.count() == 0) { return Optional.empty(); } return users.findAny(); }
Le flux, comme vous le savez, est frais avant que la terminaison ne soit appelée dessus, donc ré-accéder à la variable users dans notre code se traduira par une IllegalStateException .
Les analyseurs statiques ne savent pas encore comment signaler de telles erreurs, par conséquent, la responsabilité de leur capture en temps opportun incombe à l'examinateur.
Il me semble que l'utilisation de variables de type Stream , ainsi que leur passage en arguments et le retour des méthodes, c'est comme marcher dans un champ de mines. Peut-être chanceux, peut-être pas. D'où une règle simple: toute apparence de Stream<T> dans le code doit être vérifiée (mais dans le bon sens immédiatement coupée).
Types simples
Beaucoup sont sûrs que boolean , int , etc., est juste une question de performances. C'est en partie vrai, mais en plus, le type simple n'est not null par défaut. Si un champ entier d'une entité fait référence à une colonne qui est déclarée dans la table comme not null , alors il est logique d'utiliser int plutôt que Integer . Il s'agit d'une sorte de combo - et la consommation de mémoire est plus faible, et le code est simplifié en raison des vérifications inutiles de null .
C’est tout. N'oubliez pas que tout ce qui précède n'est pas la vérité ultime, pensez de votre propre tête et abordez de manière significative l'application de tout conseil.