Code readability - How to stop rewriting your application every 4 years

More than half of our clients come to us after a bad experience with an IT outsourcing company. And in this scenario, most of the time, we will simply maintain the existing application while we rewrite a new application from scratch. Because sadly, it's faster for us to write a new and clean app using a good framework than keep working on a poorly written system.‌‌And when talking with fellow developers, I've come to realize that restarting a new project from scratch has became a (too) common practice for many companies. And it seems to be because of the high turnover among the teams. Usually a few guys (if not just one) know perfectly the application and once they leave the company, no one is really able to work on it without breaking anything.‌‌Let's see why this happens and how we can avoid it with our current application.

The engineer syndrome

The engineer syndrome is in my opinion the main reason of an unmaintainable code base. We hate doing easy stuff. It's boring. We want to write complex code, use tricky idioms, push the language to its limit, use the latest technology, apply the coolest design pattern. Basically write cool stuff we can be proud of.

But it's important to understand that we can build complex system by writing simple code. And we should always write simple code.

Code readability is the foundation of a stable and maintainable software

By writing simple code, we allow anyone to understand and contribute to it. It's also much easier to track and fix bugs and it makes it very easy to change or create features. In this article we will go through the small things we should all do to keep our code clear and simple.

Let's start with a game

Try to list everything that is going on here. It's in Elixir but you should not need to know a language to understand what the code does.‌‌(this is a real case scenario)

def load_room(session, socket, room_code) do
    room = from(r in Room, join: p in assoc(r, :patient), where: r.code == ^code, preload: [ patient: [:staff, patient_problems: [:problem, :hospital], patient_exams: [:exam]] ] )
      |> Repo.one()
    u = Accounts.get_user_by_session_token(session["user_token"])
    is_logged = u != nil
    uid = if u != nil, do: u.id, else: nil
    :ok = Phoenix.PubSub.subscribe(Babyview.PubSub, "chat:#{room.patient_id}")
    dvs = from(d in Device, where: d.room_id == ^room.id)
    |> Babyview.Repo.all()
    wdvs = Enum.map(dvs, fn x -> %{enabled: false, device: x} end)
    msgs = from(m in Message, where: m.patient_id == ^room.patient_id)
      |> Babyview.Repo.all()
    case length(devices) do
    	0 -> {:ok, assign( socket, room: room, chat: false, messages: messages, current_user: u, devices: wrapped_devices, has_access: uid != nil, charts: %{ weight: get_weight_data(room.patient.patient_exams) } )}
	_ -> Enum.each(devices, fn d ->
        	Phoenix.PubSub.subscribe(Babyview.PubSub, "device:#{d.id}")
		Phoenix.PubSub.broadcast(Babyview.PubSub, "device:#{d.id}", {:message, "get_state"})
	      end)
            {:ok, assign( socket, room: room, messages: msgs, chat: false current_user: u, devices: wdvs, has_access: uid != nil, charts: %{ weight: get_weight_data(room.patient.patient_exams) } )}
        end
 end

We will get back to this later on.

How to write better code

Let's now list everything we can all do to keep a clean code.

Use linter and auto formater

Each language now offer plenty of linters and code formaters. And most of them comes with a default configuration that follow the best practice of the language. All your project should at least include a linter.

Define standards and stick to it

Never starts a new project without writing down the standards. Everybody has its own style of writing code. So if you don't put rules, the code base will lose consistency. For example, just knowing if inline if are allowed will drastically change the way you will apprehend the code.‌‌The few things that must be clearly define:

• snake_case vs camelCase - if you start to use both, you will very often have to check if you are expecting "userId" or "user_id". Keep one syntax.
• define keywords and avoid synonym - for example choose between 'label' or 'title' but don't use both. Same apply for 'delete' and 'remove' and many others.
• don't allow some format instead force or forbid them. For example, inline if must be written on 1 line or they must not. But developer should not have the freedom to chose to do it or not.

Simple rules like this allow readers to understand your code without having to read the whole thing. They will know where to pick up information and where to look next.

Avoid complex language idioms

Some language provide cool shortcuts to some common problems. However, most of the time, they are very hard to understand and require the reader to check online what's going and then to always keep in mind what's happening while reading the rest of the code.

Javascript provides the best examples. All the following lines are valid JS code:

if(~"foobar".indexOf("foo")) // = if("foobar".indexOf("foo") > -1)
var foo = ~~2.333 // = var foo = Math.floor(2.333)
if(foo != foo) // = if(isNaN(foo))

None of those lines really make sense. I wouldn't even know how to look it up online. They will be very confusing to anyone trying to read your code.

Write small functions

A good rule of thumb that I use is to avoid more than two level of indentation per function. Otherwise, it forces the reader to keep in my mind all the steps of your algorithm and it can quickly become confusing.‌‌Also it's good to encapsulate some tricky code into a single function with a meaningful name.

Get rid of the invalid values first

This is a small but very useful tip. Filters the values at the very beginning of your function.

Use readable variable and function names

It is something so simple yet a lot of developers still don't do it. Avoid letter or short name for variables and functions.‌‌Auto completion nowadays does wonder. Don't be scared to use long name for everything.

Write useful comments and documentation

There is no point write tons of documentation or comment every function you are writing. However comments are mandatory when:‌‌ - you are working on business logic‌‌ - you are using something not common (new library, a hidden feature of the framework, etc..)‌‌ - you are working on a complex algorithm

Split your code properly

Make sure that all your component are independent. They should not rely on each other. Instead, all your component should have clear entry point and clear output. If you are not using a typed language, make sure to comment the expected format of the parameters.

Also break your business logic into small component.

Format your code

Group the lines of code that works together and split your code by block. It will helps the reader to know when to take a break.

Avoid long lines and you can even break lines when you are listing variables.

Let's end the game

I applied everything and here is the resulting code.

def load_room(session, socket) do
    user = get_current_user()
    room = get_room_by_code()
    :ok = listen_patient_events()
    devices = list_devices_for_room(room.id)
    messages = load_messages_for_room(room.id)
    send_response(socket, user, devices)
end

def send_response(socket, room, user, devices, messages) when length(devices) == 0 do
    {:ok, assign(socket,
        room: room,
        chat_enabled: false,
        messages: messages,
        current_user: user,
        has_access: has_user_access(user),
        charts: %{ weight: get_weight_data(room.patient.patient_exams) }
    )}
end

def send_response(socket, room, user, devices, messages) do
    devices_with_states = load_and_prepare_devices(devices)
    {:ok, assign(socket,
    	room: room,
        chat_enabled: false,
        messages: messages,
        current_user: user,
        has_access: has_user_access(user),
        charts: %{ weight: get_weight_data(room.patient.patient_exams) }
    )}
end

defp get_current_user(session) do
    Accounts.get_user_from_session(session["user_token"])
end

defp get_room_by_code(room_code) do
    from(r in Room, join: p in assoc(r, :patient),
    	where: r.code == ^code,
        preload: [ patient: [:staff, patient_problems: [:problem, :hospital], patient_exams: [:exam]] ] )
    |> Repo.one()
end

defp load_devices_for_room(room_id) do
    from(d in Device, where: d.room_id == ^room.id)
    |> Babyview.Repo.all()
end

defp load_messages_for_room(room_id) do
    from(m in Message, where: m.patient_id == ^room.patient_id)
    |> Babyview.Repo.all()
end

defp listen_patient_events(event_id) do 
    Phoenix.PubSub.subscribe(Babyview.PubSub, "chat:#{room.patient_id}")
end

defp load_and_prepare_devices(devices) do
    devices_with_state = Enum.map(devices, fn x -> %{ enabled: false, device: x } end)
    Enum.each(devices, fn x ->
    	listen_device_events(x.id)
    	# We don't know the current state of a device so we trigger an event
        trigger_device_state(x.id)
    end)
    devices_with_state
end

defp listen_device_events(device_id) do
    Phoenix.PubSub.subscribe(Babyview.PubSub, "device:#{device_id}")
end

defp trigger_device_state(device_id) do 
    Phoenix.PubSub.broadcast(Babyview.PubSub, "device:#{device_id}", {:message, "get_state"})
end

defp map_device_default_state(devices) do
    Enum.each(devices, fn x -> %{enabled: false, device: x} end)
end

def has_user_access(user) when user == nil do
    false
end

def has_user_access(user) do
    true
end

This is the exact same code than before but now it is much easier to understand:

• all the variables defined have a readable name - you know their purpose
• the 'tricky' part of the code are wrapped into functions with proper naming - you don't need to know the language to understand what the line of code is doing
• we added comment to the only line of code that had no real purpose
• we use standard Elixir
• line breaks in send_response allow to easily see the final data

This conclude this article - it is very easy to write better code and it can be a real game changer for your team !

If you have a problem and no one else can help. Maybe you can hire the Kalvad-Team.