Some practical questions to ask to keep your code simple and easy to maintain

Recently I was reviewing a PR from a colleague who just started professional programming. While the PR did the job as requested by the user, the way it was implemented prompted a quick discussion which I think is worth sharing. To be fair to the colleague, he was extending an existing piece of code written by someone else. This is in no way a reflection on the programmer’s abilities.

Of course if you are a seasoned programmer, you know most of this already. This is more for people who are just starting professional programming.

Before we start, here is the piece of code when we began:

const carCodes = ['AA', 'BB']
const carList = equipmentList.filter(elem => carCodes.indexOf(elem.id.slice(0, 2)) !== -1)
const otherEquipment = equipmentList.filter(elem => carCodes.indexOf(elem.id.slice(0, 2)) === -1)

Here is where we stopped — nothing fancy and there could be more ways we can improve it further:

const isCarId = (id) => {
// AA for cars of make 1 and BB for make 2
const validCarIdPrefixes = ['AA', 'BB']
// Slicing it to 2 because first 2 characters of id is the prefix we are interested in.
const idPrefix = id.slice(0, 2) 
return validCarIdPrefixes.includes(idPrefix)
}
const carList = equipmentList.filter((equipment) => isCarId(equpiment.id))
const nonCarList = equipmentList.filter((equipment) => !isCarId(equpiment.id))

The following are some simple guidelines/questions that you can ask to see if you can make the code simpler. Typically people use DRY, KISS, Abstraction, Encapsulation and other terms but those are not easy for a beginner to relate to or how to go about implementing them in practice.

Is the intention explicit?

Can someone follow the logic of the code without having to run it mentally in their head? In the first version, I need to follow the code to understand that

1. We know a list of valid car ids.
2. We are iterating through the equipment list.
3. For every equipment, we are slicing the id for first 2 characters.
4. If that id’s index in the valid id list is not -1, then it’s a car.

Compared to the second part:

1. Iterate through the equipment list
2. See if the equipment id is a car id

Checking if id is a car id is separate logical block:

1. We know list of id prefixes.
2. We get the current id’s prefix by slicing it to 2.
3. If this list of prefixes has our prefix, then we say it’s true
4. The 2 comments make it easy for someone who come in after 2 yrs and sees this code.

This reduces the amount of information i need to keep in my head to understand the flow of logic. While this is a small thing, making what we do as explicit as possible at every point, like naming, commenting, separating parts of logic that makes it easy to understand, will be very useful when we have to change/fix something.

Are there assumptions/conventions baked in code?

A simple way to check this is “do you have hardcoded numbers/strings?” In our case, why do we say ‘2’, why only ‘AA’ and ‘BB’? Where do they come from? Will they change next month? What happens after ‘ZZ’? These are either conventions or assumptions that could change at any point in the future. If you cannot avoid hard coding, the simplest thing you can do is to add in a line of comment that says why you did it.

Are there patterns that are repeated?

Simply put “are you copy-paste-edit-ing?” In that case, even if the logic is seemingly different (like !== vs === ), you might be missing some commonality which can be extracted and represented in an elegant way.

Is there another API/function that expresses your intention clearly?

In any programming language, there are more ways to do the same thing. In this example, using “indexOf” to find out if an element exists in an Array. A simple way to check it is translating the API in a sentence and saying it out loud:

I am getting the position of this element in an array and if that position is not -1, I know that the element is not there

as opposed to using the “includes” function which reads more naturally:

If this array includes my id, I know it’s there

This is captured in the ECMAScript proposal very well:

https://stackoverflow.com/a/35370411/2231632

Which parts of the code are susceptible to change?

Typically, wherever we “state” something, instead of use logic to find something, those are the places where a code will likely change. Ironically, the parts where we are certain are the parts that most often change.

1. Hardcoded ‘AA’, ‘BB’ — there could be a 3rd and 4th valid id.
2. Hardcoded ‘2’ — it could be ‘AAA’ in 2 years.
3. carList, nonCarList — soon enough someone is going to come around asking for cars vs motorcycles vs others.

While the possibilities are huge, and it’s impossible to foresee all of them, the key point here is “when change comes, how easy it will be to make it?” We can always copy-paste-edit more times, but extracting the validation logic is one step in the way to make things a bit more manageable.

This is what people mean by ‘abstraction’ in a way that helps us manage moving parts of code without affecting the non-moving parts.

Are you over-thinking/engineering?

At every point, you should be asking ‘do i have to do all of this?’ — most of the times, the answer is a comment in code, or a ticket in your ticketing system, or adding a test case or renaming something. Some times it is refactoring. Sometimes it is re-architecting. This is the part where everyone says ‘it depends’.

Regardless of whether you implement a code change or not, you should be to answer “why your code has to be this way and not any other way”.

P.S: There are many more concerns like performance which we didn’t get into. The focus is more on readability and understandability with less cognitive overload and some tips to how to achieve them.