Cuanto más trabaja un desarrollador en una aplicación en un equipo y mejor conoce su código, más a menudo se dedica a corregir el trabajo de sus camaradas. Hoy mostraré lo que se puede atrapar en una semana en código escrito por muy buenos desarrolladores. Debajo del corte hay una colección de vívidos artefactos de nuestra creatividad (y un poco de mis pensamientos).
Comparadores
Hay un código:
@Getter class Dto { private Long id; }
Alguien señala que puede ordenar la transmisión directamente, pero quiero llamar su atención sobre el comparador. La documentación para el método Comparator::compare dice en inglés en blanco:
Compara sus dos argumentos para el orden. Devuelve un entero negativo, cero o un entero positivo ya que el primer argumento es menor, igual o mayor que el segundo.
Es este comportamiento el que se implementa en nuestro código. Que esta mal El hecho es que los creadores de Java con visión de futuro sugirieron que muchos necesitarían ese comparador y lo hicieron por nosotros. Podemos usarlo simplificando nuestro código:
List<Long> readSortedIds(List<Dto> list) { List<Long> ids = list.stream().map(Dto::getId).collect(Collectors.toList()); ids.sort(Comparator.naturalOrder()); return ids; }
Del mismo modo, este código
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; }
con un movimiento de muñeca se convierte en
List<Dto> sortDtosById(List<Dto> list) { list.sort(Comparator.comparing(Dto::getId)); return list; }
Por cierto, en la nueva versión de "Ideas" será posible hacer esto:
Abuso opcional
Probablemente cada uno de nosotros vio algo como esto:
List<UserEntity> getUsersForGroup(Long groupId) { Optional<Long> optional = Optional.ofNullable(groupId); if (optional.isPresent()) { return userRepository.findUsersByGroup(optional.get()); } return Collections.emptyList(); }
A menudo, Optional usa para verificar la presencia / ausencia de un valor, aunque no se creó para esto. Ate el abuso y escriba más fácilmente:
List<UserEntity> getUsersForGroup(Long groupId) { if (groupId == null) { return Collections.emptyList(); } return userRepository.findUsersByGroup(groupId); }
Recuerde que Optional no se trata del argumento o campo del método, sino del valor de retorno. Es por eso que está diseñado sin soporte de serialización.
métodos nulos que cambian el estado de un argumento
Imagine un método como este:
@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); } }
Seguramente has visto y escrito algo similar muchas veces. Aquí no me gusta que el método cambie el estado de la entidad, pero no lo devuelve. ¿Cómo se comportan métodos marco similares? Por ejemplo, org.springframework.data.jpa.repository.JpaRepository::save y javax.persistence.EntityManager::merge devuelven un valor. Supongamos que después de actualizar un contrato necesitamos obtenerlo fuera del método de update . Resulta algo como esto:
@Transactional public void anotherMethod(Long contractId, Dto contractDto) { updateService.updateContractById(contractId, contractDto); Contract contract = repositoroty.findOne(contractId); doSmth(contract); }
Sí, podríamos pasar la entidad directamente al método UpdateService::updateContract cambiando su firma, pero es mejor hacerlo así:
@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); } }
Por un lado, esto ayuda a simplificar el código, por otro lado, ayuda con las pruebas. En general, probar métodos void es una tarea extremadamente triste, que mostraré con el mismo ejemplo:
@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);
Pero todo se puede simplificar si el método devuelve un valor:
Asegúrate @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()); } }
De un solo golpe, no solo ContractRepository::save verifica la llamada a ContractRepository::save , sino también la corrección del valor guardado.
Construcción de bicicletas
Por diversión, abre tu proyecto y busca esto:
lastIndexOf('.')
Con una alta probabilidad, toda la expresión se ve así:
String fileExtension = fileName.subString(fileName.lastIndexOf('.'));
De eso no puede advertir ningún analizador estático, se trata de la bicicleta recién inventada. Señores, si están resolviendo un determinado problema relacionado con el nombre / extensión del archivo o la ruta al mismo, como leer / escribir / copiar, en 9 casos de cada 10 la tarea ya se ha resuelto antes. Por lo tanto, empate con la construcción de bicicletas y tome soluciones ya hechas (y probadas):
import org.apache.commons.io.FilenameUtils;
En este caso, ahorrará tiempo que se gastaría en verificar la idoneidad de la bicicleta y también obtendrá una funcionalidad más avanzada (consulte la documentación FilenameUtils::getExtension ).
O aquí hay un código que copia el contenido de un archivo a otro:
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); }
¿Qué circunstancias nos pueden impedir? Miles de ellos:
- el destino puede ser una carpeta, no un archivo
- la fuente puede ser una carpeta
- origen y destino pueden ser el mismo archivo
- el destino no se puede crear
- etc. y así sucesivamente.
Lo triste es que al usar la autograbación al respecto, todo lo que aprendemos durante la copia.
Si lo hacemos sabiamente
import org.apache.commons.io.FileUtils; try { FileUtils.copyFile(src, new File(targetName)); } catch (IOException ex) { log.error("", ex); }
entonces parte de las comprobaciones se realizarán antes del inicio de la copia, y una posible excepción será más informativa (consulte las fuentes de FileUtils::copyFile ).
Descuidar @ Nullable / @ NotNull
Supongamos que tenemos una entidad:
@Entity @Getter public class UserEntity { @Id private Long id; @Column private String email; @Column private String petName; }
En nuestro caso, la columna de email en la tabla se describe como not null , a diferencia de petName. Es decir, podemos marcar los campos con las anotaciones correspondientes:
import javax.annotation.Nullable; import javax.annotation.NotNull;
A primera vista, esto parece una pista para el desarrollador, y realmente lo es. Al mismo tiempo, estas anotaciones son una herramienta mucho más poderosa que una etiqueta normal.
Por ejemplo, los entornos de desarrollo los entienden, y si, después de agregar anotaciones, intentamos hacerlo así:
boolean checkIfPetBelongsToUser(UserEnity user, String lostPetName) { return user.getPetName().equals(lostPetName); }
entonces la "Idea" nos advertirá del peligro con este mensaje:
La invocación del método 'equals' puede producir 'NullPointerException'
En código
boolean hasEmail(UserEnity user) { boolean hasEmail = user.getEmail() == null; return hasEmail; }
habrá otra advertencia:
La condición 'user.getEmail () == null' siempre es 'false'
Esto ayuda al censor incorporado a encontrar posibles errores y nos ayuda a comprender mejor la ejecución del código. Para el mismo propósito, las anotaciones son útiles para colocar en métodos que devuelven un valor y sus argumentos.
Si mis argumentos no parecen concluyentes, mire el código fuente de cualquier proyecto serio, la misma "Primavera": están colgados con anotaciones como un árbol de Navidad. Y esto no es un capricho, sino una necesidad severa.
El único inconveniente, me parece, es la necesidad de mantener constantemente anotaciones en un estado moderno. Aunque, si lo miras, es más bien una bendición, porque volviendo al código una y otra vez lo entendemos mejor y mejor.
Descuido
No hay errores en este código, pero hay un exceso:
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());
No está claro por qué crear una nueva lista de claves mediante la cual se realiza la búsqueda, si no cambia al pasar por la secuencia. Es bueno que solo haya 5 elementos, pero ¿qué pasa si hay 100500 de ellos? Y si el método getDtos devuelve 100500 objetos (en la lista), ¿qué tipo de rendimiento tendrá este código? Ninguno, así que es mejor así:
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());
También aquí:
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))); }
Obviamente, el valor devuelto por la expresión inParameterMap.keySet() no cambia, por lo que se puede mover a una 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))); }
Por cierto, tales secciones se pueden calcular utilizando la verificación 'Asignación de objetos en un bucle'.
Cuando el análisis estático es impotente
El octavo Java hace tiempo que desapareció, pero a todos nos encantan las transmisiones. Algunos de nosotros los amamos tanto que los usamos en todas partes:
public Optional<EmailAdresses> getUserEmails() { Stream<UserEntity> users = getUsers().stream(); if (users.count() == 0) { return Optional.empty(); } return users.findAny(); }
La transmisión, como usted sabe, es nueva antes de que se llame a la terminación, por lo que volver a acceder a la variable de users en nuestro código dará como resultado una IllegalStateException .
Los analizadores estáticos aún no saben cómo informar tales errores, por lo tanto, la responsabilidad de su captura oportuna recae en el revisor.
Me parece que usar variables de tipo Stream , así como pasarlas como argumentos y regresar de los métodos, es como caminar en un campo minado. Quizás con suerte, quizás no. Por lo tanto, una regla simple: cualquier aparición de Stream<T> en el código debe verificarse (pero en el buen sentido, cortar inmediatamente).
Tipos simples
Muchos están seguros de que boolean , int , etc., se trata solo de rendimiento. Esto es parcialmente cierto, pero además, el tipo simple not null es not null por defecto. Si un campo entero de una entidad se refiere a una columna que se declara en la tabla como not null , entonces tiene sentido usar int lugar de Integer . Este es un tipo de combo, y el consumo de memoria es menor, y el código se simplifica debido a las comprobaciones innecesarias de null .
Eso es todo Recuerde que todo lo anterior no es la verdad definitiva, piense con su propia cabeza y aborde significativamente la aplicación de cualquier consejo.