Puesto que quiero que este capítulo sea lo más completo posible, a modo de referencia general de los puntos importantes, volveré a exponer ejemplos anteriores. Mostraré ejemplos de lo que no se debe hacer y, a continuación, lo que creo que es correcto. Empecemos:
No comentar cosas evidentes o triviales
/**
* Returns the id
*
* @return The id
*/
public int getId() {
return id;
}
public int getId() {
return id;
}
Explicar cosas no evidentes
if (!jobs.isEmpty()) {
Job job = jobs.getNext();
job.execute();
}
if (!jobs.isEmpty()) {
// Not all the jobs will be in "jobs", depending on general config,
// some jobs will be also in "otherJobs"
Job job = jobs.getNext();
job.execute();
}
Usar siempre nombres de variables explicativos, aunque sean auxiliares
Dependiendo de lo lejos que esté el bloque «if», e incluso estando al lado, solo viendo el código necesitas analizar qué está haciendo para saber qué está comparando. Un simple comentario y dos nombres de variables más adecuados, y ya no tendrás que analizar nada.
const p1 = product.properties.map(a => Object({id:a.id, attribute: a.attribute}));
const p2 = myProperties.map(a => Object({id:a.id, attribute: a.attribute}));
(some lines below...)
if (JSON.stringify(p1) === JSON.stringify(p2)) {
product.stock++;
}
// Get current product properties and my chosen properties and force the same order/format to compare later
const currentProductProps = product.properties.map(elem => Object({ id: elem.id, attribute: elem.attribute }));
const myChosenProps = myProperties.map(elem => Object({ id: elem.id, attribute: elem.attribute }));
(some lines below)
if (JSON.stringify(currentProductProps) === JSON.stringify(myChosenProps)) {
product.stock++;
}
Usar nombres de funciones correctos
Aquí ves dos ejemplos opuestos de malos nombres de funciones: uno es demasiado largo, que añadido a su paquete es casi como leer un libro; el otro es demasiado corto (¿calcular qué?). Mejores nombres hubieran sido, por ejemplo:
public getAuthenticationDetailsFromPreAuthenticatedUserDetailsSource() public calculate()
// Explain here the details of the function, not in the name public class getAuthenticationDetails() public calculateSalary()
Describir las variables no evidentes
Hay variables que son evidentes, pero otras pueden ser más complejas de lo que aparentan. Describir su cometido ayuda a entender mejor el contexto.
export class GridListComponent {
public callingAPI = false;
private gridApi: GridApi;
public columnDefs: ColDef[] = [];
public rowData: Array<Record<string,unknown>> = [];
public gridOptions: Record<string,unknown>;
public selectedRow: any;
public fromDate: string;
public toDate: string;
(...)
export class GridListComponent {
// Show spinner while checking data
public callingAPI = false;
// Ag-Grid API
private gridApi: GridApi;
// Define here the columns and configs for the ag-grid ([])
public columnDefs: ColDef[] = [];
public rowData: Array<Record<string,unknown>> = [];
// Special properties here, like localization
public gridOptions: Record<string,unknown>;
public selectedRow: any;
// Optional user inputs for searching between dates
public fromDate: string;
public toDate: string;
Explicar todo lo que sea necesario, aunque ocupe varias líneas de comentarios
Como programador recién llegado al proyecto, no tienes el contexto para entender por qué el modo multipuesto usa datos del servidor y el monopuesto no los usa. ¿O es un error del programador y nadie se ha dado cuenta aún? Tampoco sabes por qué en la línea 27 se emite una notificación: ¿es necesaria siempre que se actualiza un producto? ¿O solo con el stock? ¿O solo en este momento determinado?
this.apiService.add('order', this.ticket).subscribe(
(res: GenericObjectReponse) => {
this.ticket.ref = res.data.order.ref;
this.ticket.createdAt = res.data.order.createdAt;
let productModified = false;
if (this.globals.multistationMode && res.data.products) {
res.data.products.forEach((product: Product) => {
if (product.stock.stockType != 'disabled') {
productModified = true;
this.apiCachedService.update('product', product, product.id);
}
});
} else {
this.ticket.orderLines.forEach(line => {
if (line.product.stock.stockType != 'disabled') {
productModified = true;
line.product.stock.units -= line.units;
this.apiCachedService.update('product', line.product, line.product.id);
}
});
}
if (productModified) this.notificationService.emitUpdatedProducts();
}
);
La clave de la línea 27 es que actualiza una vista llamada «product-box», que ahora sí, el programador sabe que contiene el dato visible del stock y que, por tanto, debe actualizarse para que el usuario final pueda verlo correctamente actualizado.
this.apiService.add('order', this.ticket).subscribe(
(res: GenericObjectReponse) => {
// Depending on multistationMode, the behaviour will change:
// single -> All locally managed, we only expect data about the order. Server will process product stock by itself internally
// multi -> We wait for order and products, because other user could modify products stock, so only the server has the right stock
this.ticket.ref = res.data.order.ref;
this.ticket.createdAt = res.data.order.createdAt;
// Update cached product stock, if necessary
let productModified = false;
if (this.globals.multistationMode && res.data.products) {
res.data.products.forEach((product: Product) => {
if (product.stock.stockType != 'disabled') {
productModified = true;
this.apiCachedService.update('product', product, product.id);
}
});
} else {
// Single mode, update products locally with our own data
this.ticket.orderLines.forEach(line => {
if (line.product.stock.stockType != 'disabled') {
productModified = true;
line.product.stock.units -= line.units;
this.apiCachedService.update('product', line.product, line.product.id);
}
});
}
// Send the signal so product-box view can update products
if (productModified) this.notificationService.emitUpdatedProducts();
}
);
Explicar conceptos complejos
¿Crees que trabajar con sockets es sencillo? ¿Y con mensajes HL7 (estándar de intercambio electrónico de información clínica)? El código de arriba es muy específico y se encarga de leer cualquier información enviada a un socket que está escuchando. ¿Crees que un novato la entendería solo con ver el código? ¿Y tú mismo, dentro de un año o dos dedicado a otros proyectos? Hagámoslo más fácil de comprender. Este código me lo encontré tal cual está en un proyecto sin haber tocado nunca antes sockets. Los siguientes comentarios los agregué yo para no tener que perder tiempo analizando esto mismo, de nuevo, en el futuro.
public static StreamTreatment readHL7MessageFromLLP(BufferedReader readSocket, String charsetToUse, boolean runParser, String configKey) throws UnsupportedEncodingException, IOException {
StreamTreatment receiveStream = null;
String receiveString;
boolean cicleFlag = true;
while (cicleFlag) {
receiveString = new String(readSocket.readLine().getBytes(), charsetToUse);
if (receiveString != null && receiveString.length() > 0) {
if (receiveString.charAt(0) == (char) Integer.parseInt("0B", 16)) {
receiveStream = new StreamTreatment();
receiveStream.addStream(receiveString.substring(1));
} else if (receiveString.charAt(0) == (char) Integer.parseInt("1C", 16)) {
receiveStream.parseMessage(runParser, configKey);
cicleFlag = false;
} else {
if (receiveStream != null) receiveStream.addStream(receiveString);
}
}
}
return receiveStream;
}
/**
* Explanation:
* readSocket.readLine().getBytes() will wait for input stream, once the client is connected. At that moment, a few options can happen:
* 1. Just an open connection with no data: the loop will wait forever
* (if persistence enabled) o until SocketTimeoutException is thrown (if no persistence mode).
* 2. Some data is sent over the socket:
* 2.1.- If it's a valid HL7 encapsulated message, will be returned in ivReceiveStream and processed
* 2.2.- If it's garbage, will not be processed and connection will never be closed until the stream finish.
* Once the stream finish, if persistence is ON, the connection will still wait for a valid HL7 encapsulated
* message. If no persistence, SocketTimeoutException will be thrown after the last character + configured timeout
*/
public static StreamTreatment readHL7MessageFromLLP(BufferedReader readSocket, String charsetToUse, boolean runParser, String configKey) throws UnsupportedEncodingException, IOException {
StreamTreatment receiveStream = null;
String receiveString;
boolean cicleFlag = true;
while (cicleFlag) {
receiveString = new String(readSocket.readLine().getBytes(), charsetToUse);
if (receiveString != null && receiveString.length() > 0) {
if (receiveString.charAt(0) == (char) Integer.parseInt("0B", 16)) {
// New message received and it is HL7 because starts with "0B" hex
receiveStream = new StreamTreatment();
receiveStream.addStream(receiveString.substring(1));
} else if (receiveString.charAt(0) == (char) Integer.parseInt("1C", 16)) {
// End message of HL7 because ends with "1C" hex
receiveStream.parseMessage(runParser, configKey);
cicleFlag = false;
} else {
// This could be part of HL7 message or garbage
if (receiveStream != null) receiveStream.addStream(receiveString);
}
}
}
return receiveStream;
}
Explicar soluciones que se probaron pero que finalmente no sirvieron
Supongamos una aplicación que trabaja todos sus datos desde una caché. Por regla general siempre se recargan de ahí, sin embargo en un momento determinado no es así: ¿es un fallo? Mejor explicarlo.
function reload() {
loadDataModel(user);
}
function reload() {
// Load new user data from source
// Do not use loadCacheModel() because cache is invalidated by previous function call in other module.
loadDataModel(user);
}
Otro ejemplo diferente en el que se descubrió una mejor y más eficaz forma de afrontar un problema al que ya se le había dado solución previamente:
constructor(public router: Router) {
// Changing between routes within same Component will not fire any cicleLife event (ngOnInit, ngAfterViewInit, ngOnDestroy...) by default
// Force it by disabling routeReuseStrategy.shouldReuseRoute
this.router.routeReuseStrategy.shouldReuseRoute = () => false;
// Not needed now because the new discovered "routeReuseStrategy.shouldReuseRoute" disabling strategy, but for future references I leave code here
// // Because changing between routes with same Component will not fire ngOnDestroy, check for route change to unsusbscribe from update error
// // to avoid multiple subscriptions on every screen change between models, instead do it onDestroy
// this.router.events.subscribe((event: any) => {
// if (event instanceof NavigationStart) {
// if (this.subscription) this.subscription.unsubscribe();
// }
// });
}