Nathaniel at Engineering here – when working on the Universal Layer LLC Customer Panel one of the challenges that came up was writing logic that was very complex while remaining clean and readable. It’s very easy to write code that while it works, is very messy and hard to read. Style is just as important as functionality if you plan on making changes later on.
Crystal’s compiler is very strict, to avoid runtime errors later on
One of the benefits of switching to Crystal Language, as well as a challenge to writing code in it, is that the compiler is very strict and does a lot of checks on your code before it compiles. This prevents a lot of common runtime errors that could occur with the same logic in Ruby.
A messy reset method for Virtual Machines
Consider the following method, it has a lot of things to do before we’re sure it’s safe to reset a Virtual Machine. It has to do the following:
- Get a reference to the
current_user
entity, every time an entity is changed it’s copied to a new space in memory, the old entity is still available if a variable that references it. Once all references to the old entity are gone, the garbage collector will delete the data and allow the space in memory to be used again. Since thecurrent_user
entity is subject to change at anytime that code is running, we need to get a reference to the current point in time. We have to do the same with getting the user’s ID number (due to the implementation it’s unlikely to change, but semantics matter a lot while programming). - Get a reference to the
VirtualMachine
entity. This contains various information about the machine so we can contact the correct hypervisor and reset the Virtual Machine. - Get a reference to the
service
entity. Theservice
entity stores information about billing status, any locks on the service, among other things that could apply to entities other thanVirtualMachines
. - Check that there is not a
read_only
status on theservice
(we also check if aservice
is disabled, terminated, etc – it was removed from this example to keep it relatively simple). - Request the
VirtualMachine
entity’sreset_vm
method to launch. The entity has it’s own methods and logic to make this work for us and avoids rewriting a lot of code. For example a ULayer employee might need to help a customer who contacts us and resetting the Virtual Machine will be one of the basic troubleshooting steps we take. Writing the logic twice makes the code messier and harder to change later. We’re discussing internally whether status checks to prevent an action should be addressed by the entity or the controller. We could add an optional parameter to overwrite the check. For now though, we’ve chosen to address this in the controller. - Create an alert (internally this system is called flash messages)
- Redirect the user. During the alpha all redirects went to the homepage, and improvements are ongoing to our use of redirects.
def reset
if user = current_user
if user_id = user.id
if server = VirtualMachine.find_by(user_id: user_id, id: params[:server_id])
if service = Service.find_by(id: server.service_id)
unless service.service_is_read_only
server.reset_vm
flash[:info] = "Server reset!!"
redirect_to "/"
end
end
end
end
end
end
Needless to say this code is a bit messy. What happens if we need to start referencing and using other entities or adding in extra logic to this method? Making this readable becomes a top priority.
Our solution
We found a really neat alternative to nested if-statement logic we were using. Similar situations with lots of functions relying on each other can occur in JavaScript, commonly referred to as “callback hell” and this reminded the team of it. Consider the following code:
def reset
if (user = current_user) && (user_id = user.id) && (server = VirtualMachine.find_by(user_id: user_id, id: params[:server_id])) && (service = Service.find_by(id: server.service_id))
unless service.service_is_read_only || service.service_is_disabled
server.reset_vm
flash[:info] = "Server reset!!"
redirect_to "/services/virtual_machines/#{server.id}"
end
end
end
This new approach creates a single if-statement, we evaluate several isolated expressions (and they’re evaluated in order using the &&
operator), if they all evaluate to a truthy value (e.g. not false or nil), the code block runs (we will return an error screen in the event of a failure in a future version of our application). We still decided to nest an unless-statement (unless-statements run unless the expression evaluates to a truthy value. It’s easier to read than the if-statement !expression (you see to accomplish this in other languages) that checks the service status flags. Since we’re considering moving the service status flag checks into the VirtualMachine entity this design decision makes our code readable and easier to change later on.
Drawbacks
No solution to writing clean code is perfect. Some of the drawbacks of our approach include:
- The code is more readable and avoids nested if-statements. That said the expression evaluation chain is quite long and could still be challenging to read later on.
- Several expressions must evaluate before the code block is run. In most cases they evaluate quickly however if we wanted to call in data from a remote API, execution of the code block would be deferred until data was returned.
- We might want to move service status flag checks to the entity, although there are several considerations to make before we’re ready to do this.
Conclusion
There are many things to consider when writing clean code and it varies depending on the purpose of the application and the applicable logic. We’re happy with the success our current approach has given and look forward to improving it even more. In the meantime, check out our panel :).
What I usually do in this case is use the `try` method like so:
“`
def reset
current_user
.try { |user| VirtualMachine.find_by(user_id: user.id, id: params[:server_id]) }
.try { |server| Service.find_by(id: server.service_id) }
.try do |service|
unless service.service_is_read_only
server.reset_vm
flash[:info] = “Server reset!!”
redirect_to “/”
end
end
end
“`