Since I want this chapter to be as complete as possible, by way of general reference of the important points, I will re-set out previous examples. I’ll show examples of what not to do and then what I think is right. Let’s go:
Don’t comment on obvious or trivial things
/**
* Returns the id
*
* @return The id
*/
public int getId() {
return id;
}
public int getId() {
return id;
}
Explain unobvious things
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();
}
Always use explanatory variable names, even if they are auxiliaries
Depending on how far away the “if” block is, and even being next to it, just by looking at the code you need to analyze what you’re doing to know what you’re comparing. A simple comment and two more suitable variable names, and you won’t have to analyze anything anymore.
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++;
}
Use correct function names
Here you see two opposite examples of bad function names: one is too long, which added to your package is almost like reading a book; the other is too short (calculate what?). Better names would have been, for example:
public getAuthenticationDetailsFromPreAuthenticatedUserDetailsSource() public calculate()
// Explain here the details of the function, not in the name public class getAuthenticationDetails() public calculateSalary()
Describe unobvious variables
There are variables that are obvious, but others may be more complex than they appear. Describing their role helps you better understand the context.
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;
Explain everything you need, even if they are several lines of comments
As a newcomer to the project, you don’t have the context to understand why multistation mode uses server data and monostation doesn’t use it. Or is it a programmer’s error and no one has noticed yet? You also don’t know why a notification is issued on line 27: is it necessary whenever a product is updated? Or just stock? Or just right now?
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();
}
);
The key of line 27 is that it updates a view called “product-box”, which now the programmer knows that it contains the visible stock data and that, therefore, it must be updated so that the end user can see it correctly updated.
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();
}
);
Explain complex concepts
Do you think working with sockets is simple? What about HL7 messages (standard for electronic exchange of clinical information)? The code above is very specific and takes care of reading any information sent to a socket you are listening to. Do you think a rookie would understand it just by looking at the code? And yourself, in a year or two dedicated to other projects? Let’s make it easier to understand. I found this code as it is in a project, without ever touching sockets before. The following comments were added by myself so that I don’t have to waste time analyzing this myself, again, in the future.
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;
}
Explain solutions that were tested but ultimately did not work
Suppose an application that works all its data from a cache. As a general rule, they always reload from there, however at a certain moment it is not like this: is it a failure? Better explain it.
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);
}
Another different example in which a better and more effective way to deal with a problem that had already been addressed was discovered:
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();
// }
// });
}