Polymorphic method signatures
April 23, 2016
1 min 55s
A few days ago, I was working on a new feature at work. The specifics aren’t important, but it’s worth mentioning that they involve object-oriented inheritance.
There was a method that all the classes shared, which takes a single argument and returns a boolean value. In the case of the base class, it was hardcoded, and therefore never used the argument. For that reason, the argument was made optional.
One class overrode this and did use the argument, though it kept it optional and opted for some confusing branches when the argument is not passed in. I was overriding this method for another class, and I absolutely required the argument.
Well, that caused some buggy behaviour, because some places in the codebase were already calling this method without an argument. I was forced to change my version of the method to accomodate a call without the required argument.
Of course, that wasn’t sufficient, because the line causing the issue wanted the opposite behaviour without passing in the argument, but there were some security flaws with doing it that way (you can probably guess what we were passing in, and probably what the method does now, which I was trying to avoid, because it’s not great, but eh…).
Luckily, in this case, I could just change the line causing the issue to take in the argument, but there is at least one place in the codebase that almost couldn’t have happened (I could easily see a version of that part of the codebase that just did not have the argument we were looking for, and it’s from a gem, so it would have been bad to change).
It probably never occurred to whoever wrote the original base method, but here are two things to take away from this:
- Keep your shared method arguments consistent. If an argument is optional somewhere, it should be optional somewhere else.
- Probably don’t use optional arguments with base methods in the first place, as you never know when that argument will be required for an overriding class.
What makes code good?
March 27, 2016
10 min 43s
My brother recently asked me what makes code good. There are so many different factors that go into making a piece of code good. I am also going to talk a little about beautiful code. I think beautiful code has some overlap with good code, but not completely. I will distinguish between goodness and beauty (which is a subset of goodness). Here’s how Google defines ‘beautiful’ and ‘good’:
beautiful
/ˈbjuːtɪfʊl,-f(ə)l/
-
pleasing the senses or mind aesthetically
good
/gʊd/
-
to be desired or approved of.
Many of these virtues feed into each other, sometimes one helping the other, sometimes hindering it. Many of the less important virtues are there to support the more important virtues by making it easier for the programmer to manifest them, which in turn are there to support the most important virtues (which will, of course, always depend on what your goals for the project are, though, usually the same ones appear near the top of the list).
So, in some vague half-order, here are some of the different factors that go into making good code.
Function
The most important question is, does it work? As a programmer, this may be the most boring requirement of good code on the list, but I believe it’s the most important. Functional code is good code.
Even if you were only looking at a piece of code to appreciate any of the other qualities and didn’t care at all about the resulting program, if you know it doesn’t work, all the other qualities stand for nought (one exception: examples highlighting use cases for feature proposals to a language). It’s easy to create code that adheres to strict standards of goodness if there’s no requirement that they work.
For the users of the code, this is the most important quality, as it gives meaning to the program. For the (insular) programmer who doesn’t care about the program, this is the most important quality, as it gives meaning to the code and the challenge faced in producing it.
Correct code is good code. Isn’t this just the same as above? Well, no, not exactly. There’s a quote attributed to Kent Beck that goes a little something like this:
Make it work, make it right, […]
That “make it right” means different things to different people, but one possible interpretation is to say that, “make it work” means to make the program work in the ‘happy path’ and “make it right” means to cover edge cases.
So, a functional program (not to be confused with functional programming) is a program that does its job in the happy path. It’s useful, even a little bit, and serves some loosely defined purpose. By contrast, a correct program is one that adheres to some more precise specification that covers what the program should do in non-happy cases.
This is a much more difficult (and currently impractical) endeavour, even if we could do it, and is a whole subfield of computer science.
Maintainability
Maintainable code is good code.
If the program works, it may become very successful (i.e., it achieves its goals very well). If it becomes successful, it will probably have a long life serving the needs of its users, and those users, now more successful with whatever their goals are with the help of the program, have new needs due to their newfound success. They may want to increase the scope of the program. This is not a bad thing, but you should be careful about scope creep.
A maintainable program is one that’s easy to change to fit new requirements that are likely to come up. Not only that, but it’s possible to change without sacrificing maintainability. By that, I mean that if the program is easy to change to fit new requirements, but changing it decreases maintanability, then it wasn’t really that maintainable to begin with.
Readability
Readable code is beautiful code. It’s code that can be read… and easily understood. It’s best if a non-programmer can look at the code and at least sort of have an idea of what’s going on. Readable code is more maintainable code (usually).
However, it’s important not to target non-programmers when deciding between something that is more readable to a non-programmer vs. something that’s more readable to a programmer who takes the time to understand abstract concepts (you should know your reader and strike a balance). Software development is all about abstraction. They make our lives easier, as programmers and non-programmers.
Expressivity
Expressive code is beautiful code that clearly expresses the author’s intent to the reader. Expressive code is readable code is maintainable code and expressive code is also clear code is maintainable code.
For example, when fixing bugs, oftentimes, you may be reading a code fragment and it looks like the author made a particular decision for the code to perform its task a certain way. If it looks like the author was very sure in their intent to do it the ‘buggy’ way, you may wonder whether fixing the bug may introduce other errors that the original author was trying to avoid. Incidental trivia should look incidental, and purposeful decisions should look purposeful.
Unfortunately, this particular example and others like it are quite tricky to avoid, but it’s part of our job as programmers.
Concision
Concise code is beautiful code. If I had to choose between the two pieces of code below:
numbers = [1, 2, 3, 4, 5].map { |i| i + 1 }
numbers = [1, 2, 3, 4, 5]
numbers.each_with_index do |number, index|
numbers[index] = number + 1
end
I’d choose the first one in a heartbeat. Although, after learning some Haskell, that ugly block syntax for such a simple lambda makes me queasy. Haskell does this much more beautifully: numbers = map (+ 1) [1, 2, 3, 4, 5]
. Mmm…
Despite the fact that the second one is more readable to someone who has no idea what map
is about, to someone who does, the first is much more readable, as the abstraction of map
is ingrained in them, and they instinctively understand what you’re doing without having to read such boilerplate that could subtly change.
Avdi talks about making same things look the same, and different things look different. Abstractions like map
(applying a function to all elements of a list, returning a new list with the elements replaced by the result of the function application) help us do that.
Being too concise hurts readability and expressivity. At a certain point, the code just becomes inscrutable. You don’t want write-only code. There isn’t a hard line to draw, just know your reader, nearby code, language/community idioms, and other contextual factors.
Simplicity
Simple code is beautiful code. Simplicity is about the right level of abstraction. Abstraction is a useful tool, but not all abstractions are equivalent. There is such a thing as too much abstraction (indirection/overengineering) or the wrong abstraction or doing something at the wrong level of abstraction or too little abstraction.
Done well, abstractions should encapsulate something complicated into something simple (but not something complex into something simplistic). Abstractions can build on each other, but be careful you don’t add unnecessary layers or complicate the whole system in your pursuit to simplify the parts. Those parts will need to work together, and if they’re broken up into unnecessarily small pieces, the – possibly repetitive – glue code could end up being what complicates things.
Speaking of repetition…
DRYness
Good code should be DRY. DRY stands for Don’t Repeat Yourself and DRY code is nonrepetitive code, and is a huge part of the reason we program in the first place. The first virtue of a great programmer is laziness, “the quality that makes you go to great effort to reduce overall energy expenditure”. One of the great strengths of programming is the ability to delegate tedious, repetitive tasks to the machine.
Well, while automating, we often encounter repetitive tasks that we need to do to support the architecture and building of the automation, so what do we do? Why, automate them of course. There exist many abstractions to reduce duplication in code, and many tools to reduce duplication in programming effort.
Performant code is good code. Most of the time, performance isn’t a huge deal, because whatever you write will probably be faster than whatever manual process it takes over, or not significantly slower than whatever automatic process it takes over (from the human’s perspective, if you do it right). That said, users do expect a lot from a machine nowadays, and a web page that takes 2 seconds to load, for example, is probably at least 1.5 seconds too long. Spending too much time on this and on the wrong parts of the codebase is considered premature optimisation (which, as a concept, could also be applied to other qualities of good code).
In some contexts, like automated trading systems, performance is critical, and really is part of the functionality, as there are minimum performance characteristics required for the program to be successful.
Most of the time, when it comes to performance, [you should only care about the low-hanging fruit.
Cleverness
Clever code has a certain beauty to it for software developers, this is like candy to us. Unfortunately, cleverness is often at odds with reliability, as it may rely (foolishly) on private APIs, low-level tricks, and other unspecified or unguaranteed behaviours.
Reliability
Good code is reliable, rock solid, and doesn’t break. This is not just about adhering solidly to a spec, like correctness, but rather about the spec itself being solid and anticipating as many practical edge cases as reasonably possible.
I guess this is not so much about code itself, but good code should be an embodiment of its spec (correct), and if the spec leaves a lot undefined, good code should account for that, anyway, while still adhering to the spec where it is defined. So, the code itself should be reliable, even if the spec doesn’t force it. This means being liberal in what you accept, and conservative in what you send.
Aesthetically and ergonomically pleasing code is beautiful code. This is probably one of the easiest ones for non-programmers to appreciate. It’s easy to navigate with your editor and easy to scan with your eyes, or whatever you use to consume code.
Ergonomically pleasing code is more maintainable, as it’s easy to navigate, and aesthetically pleasing code is more readable.
Consistency
Beautiful code is predictable, in form, as well as function. Consistent code is also more maintainable. Combine this with good form, and you have a winner. However, consistency is more important than form itself. I can’t think of a single rule of good form that is worth being inconsistent for. Code that adheres to best practices followed by a language or framework’s community is said to be idiomatic.
The reason consistent (and aesthetically pleasing) code is more maintainable is that it’s easy to search through, and easy to record editor macros for. Yep, more automation. Adhering to more rigid standards helps the reader scan the code easier, and helps when searching with the computer too, as you don’t need to search for such complex patterns (yep, simplicity rearing its beautiful head again).
The Case of the Stubbornly Incomplete Booking
January 13, 2016
6 min 46s
The other day, I was at work (as you do), and I felt like Sherlock. I was working through a bug in the system, and a puzzle piece just clicked and I felt good, and I felt compelled to put it out there, even if no one is reading this.
This is a bit of a complex issue, so strap yourself in. It’s made a little more complex by the fact that you, dear reader, do not have the shared context that my coworkers did (unless you are a coworker).
I work for a company called SalesMaster, and we offer a pretty sweet showroom app for car manufacturers and retailers (if I do say so myself). Users of our system can create a record representing an enquiry that can have various reasons for existing. One of these types of enquiries is the ExperienceBookingEnquiry, which is an enquiry that gets created for a manufacturer who offer experience days (customers can spend a day driving a similar vehicle off road in a dedicated centre). A booking can then be attached to this enquiry.
Experience bookings, as well as their enquiries, can be in a finite number of states that, taken together, represent a workflow. We use a gem called “workflow” to manage this workflow. This particular gem has a hook that allow the programmer to trigger events when entering states.
The following two figures represent the states that an experience booking and experience booking enquiry can be in and the states it can move to from there. An arrow pointing from state A to state B means that the record can move to state B if it’s in state B. The left-most state is the one that the record starts in (don’t ask me why the booking starts off at “confirmed”).
Figure A: experience booking enquiry
New ----> Booked ----> Completed
^----------/ \---> Declined
Figure B: experience booking
Confirmed ----> Completed
\---> Cancelled
Here are the trigger-like behaviours to note:
- An experience booking enquiry is moved to the “Booked” state when an experience booking is created on it (but not through triggers, we just do this one through an interactor).
- When the booking enters the “Completed” state, it tries to move its associated enquiry to the “Completed” state as well.
- When the booking enters the “Cancelled” state, it tries to move its associated enquiry to the “New” state (but only if the enquiry is in the “Booked” state, so it won’t try to move an enquiry from “Completed” to “New”).
Also of relevance is the fact that an enquiry can have multiple bookings made against it. Quicker readers that are willing to assume may see where I’m going with this already, and they’d probably be right, but they wouldn’t have the full story with the human and luck elements to it. If you’re interested in that, stick around.
I had been working on a bug that could be described as follows: some experience bookings were unable to be moved to the “Completed” state, which I figured out was because the enquiry would refuse to progress to “Completed” with it. It turned out that these enquiries were left in the “New” state, when they should have been in the “Booked” state (clearly, as they had an associated booking). Trying to figure out why these enquiries weren’t in the “Booked” state proved fruitless. Looking at the interactor:
if booking.save
progress_enquiry booking.enquiry
I thought, maybe the booking would succeed, but the enquiry would fail to move to the booked state. Of course, it was hard to diagnose why at this point, as I did not have the error logs from this occurrence (which never occurred). Eventually, I decided to wrap the thing in a transaction and just note on the PR that I hadn’t attacked the problem directly (which I should have done in the commit message and been clearer about).
We have a killer feature, which is that you can view bookings in a diary view that shows the vehicles they belong to and the time they span in a very efficient visual manner. You can also drag and drop bookings to change their time, and even their vehicle. When dragging between vehicles, the app actually creates a whole new booking from the template of the old, then cancels the old one. Here’s where luck comes into play: the sprint contained another bug to do with bookings, which was that very occasionally, some bookings, when moved to another vehicle (i.e. cloned and cancelled) would then have their VRMs (number plate) point to the new vehicle on the old, cancelled booking. If this bug weren’t on my mind, I would have never made the connection that I did.
I spent some time on that bug, then came back to the previous bug when a coworker, while QA testing, asked for some guidance on expected behaviour. Now, this was an unusual QA test. Because I had to simulate the exception being raised when the enquiry is moved to the “Booked” status, thereby leaving a booked enquiry in the “New” status, I could only do it through automated testing. That test is a less interesting story in itself. It would be impossible for a human to test without overriding the method to move the enquiry to “Booked” to raise an exception, then changing it back for production. I asked QA to test that I at least hadn’t broken anything.
What my colleague had needed answering was whether the enquiry was supposed to move back to the “New” state when the booking is cancelled. Oh! A little dramatic irony; all that stuff about workflow that you now know? I wasn’t thinking about any of that while originally working on the bug. We used workflow hooks to trigger events in some cases, but used an interactor for that one case I mentioned. It was also originally triggered through a workflow hook, but had probably been moved to the interactor to keep it self-contained and easy to reason about. Ironically, that probably misled me into believing that I was look at all I needed to, when there were other moving parts that were more pertinent.
It was expected behaviour, but I noticed the app wasn’t checking to see if the enquiry had other bookings on it before moving it to the “New” state. It was at this point I spotted the issue: a booking was being created on an enquiry, then another on the same one. One of them was then being cancelled, moving the enquiry to “New” and leaving the other booking high and dry to be unable to complete. After a colleague I had requested help from asked why users were doing this (as we happened to know this was not supposed to be the process for the manufacturer who reported the issue), a second later, I recalled the issue I had just arrived from working on and realised that users were dragging the booking on the diary view. Some investigation on the affected bookings, including inspection of the activity log, we confirmed this with reasonable certainty.
Maybe anticlimactic, but it certainly gave me a small rush at the time. Feel free to email me at habib@alaminium.me if you liked this story and want to hear more like it. I would welcome it.