Before starting development on a legacy project, we are often asked to do a quick review of its quality. Some projects are easy to analyze and the task just boils down to a look into a couple of classes, routes file, tests etc.; yet from time to time we realize that the actual problems might be harder to discover than we thought. There is also a situation in which we do not have access to the codebase before we sign an agreement and start to work on the project. In such a situation it may pay off to establish the key practices that the development team might have (or might not have) followed. Those practices can not only give us some quick and precise feedback on the project quality but they can also serve as a checklist for keeping our existing and new projects healthy.
“Test Driven Development” features in almost all programming-related job postings, at least in the Ruby on Rails ecosystem. But do we actually do it? And to what extent? What is our coverage? How much does it cost to maintain a green test suite? Those are all valid questions if we intend to follow routines that keep our project in a healthy shape.
It is not impossible to write even medium-sized projects without specs at all or cover tiny projects with hundreds of tests with minimum value. One’s job is to find that sweet spot in which our tests bring a lot of value while not taking us many extra hours of work just to maintain the suite after each small change. Pure TDD is also something that can bring us problems in application/architecture design (the so called Test Induced Design Damage, that DHH has some great talk about), when our drive to make the code testable makes it less readable and less comprehensible.
It might be a good idea to consider the Outside-In Testing approach to keep our testing practices under control. In this case development is driven by high-level tests (i.e. feature specs) and then each lower-level concern or edge case is handled by more specific tests. Thus we do not end up testing the same thing multiple times and with some occasional experimentation before “TDDing” we can end up with a pretty decent architecture.
All in all, the most basic question is about tests presence. If there are no/few tests in the codebase it is a fairly reliable indication that you may have to face some significant / challenging problems.
Well, the actual question should be “Do you use design patterns to keep your code quality high?”. There is no use for design patterns where there is no place for them, so those should only be applied consciously. Times when “you should keep your controllers slim and models fat” are long gone now. Models (or in fact most classes) do deserve to be kept slim as well. It virtually boils down to keeping each class responsible for only one thing — the so called Single Responsibility Principle.
The decorators and service objects, mentioned in an old yet still valid post by @brynary about the slimming of models, became quite popular, but that is not all we can do to keep our classes testable while keeping their responsibility narrow. “Design Patterns in Ruby” (by Russ Olsen) is a great source for diving into more sophisticated solutions and extending our skills toolbox with some nifty techniques.
Leaving design patterns aside, it deserves to be mentioned, that keeping our controllers actions limited to the RESTful ones can also help in keeping those classes in good shape. DHH himself has some strong opinion about it, as it is outlined in this post by Jerome Dalbert. Also following Sandi Metz Rules for developers should do some good to the shape of our classes.
To sum up, the point is not to use as many design patterns as possible, the goal is to keep our classes slim, with narrow responsibilities and public interfaces. If a design pattern is a means to an end here, let’s use it! If we add a bit of refactoring here and there in the classes themselves to keep the method complexity low, we will end up with comprehensible, reusable and testable code at the cost of added indirection (which a decent coder can deal and live with). At least, this is the plan. On the other hand, if the project is of a moderate size and using few design patterns and/or class slimming techniques, we can expect some of the classes to be huge code dumps, which are not very comfortable to work with. This might not be true for projects with very simple logic and requirements, yet it is a warning sign we should be aware of.
Ok, so there we have this magic code — almost no code there and everything just works! Awesome! Well sort of. There are so many means to keep our code slim (do not mistake with slim classes) — gems, libraries, convention-over-configuration-everything! It looks nice at the beginning , yet later it turns out we need to customize something (especially something not foreseen by the library author), or somebody else is to work with our code. The “magic” now becomes a problem as it makes a given implementation harder to understand, requires diving into multiple libraries, crafting customized solutions to modify the default behavior. The code looks nice, but it is not easy to work with. The key is to decide if a given solution really gives us much benefit in terms of less code being written, or is it ok to just write a couple of lines and keep code comprehensible and easily modifiable. After all being explicit about a given behavior is advantageous in most cases.
In Ruby on Rails (or for that matter in Ruby itself) there is also this very tempting thing that drives “magic” to a new level. Metaprogramming. When you discover it, you just have to love it. This love dies quickly if it was not you who coded the piece. Dynamically creating methods or altering their behavior makes tracking what code does a living hell and renders stack traces useless. I may be exaggerating a bit, but it definitely can make things much more complex to understand.
There is one more place where we might pay even more attention as regards the trade-off between readability and being DRY (Don’t Repeat Yourself). Specs. One might find that keeping tests very DRY makes them much less readable; e.g. when we need to jump around a file to locate those “shared” things, aggregate them in our head, find what the subject is all about and then finally what is tested in those “shared examples” and “shared contexts” — the word “shared” becomes our foe. Keeping test prerequisites/setup, executing code being tested and asserting the result all in one place renders the test (more) comprehensible, yet it does so at the cost of repetition, the cost we are ready to pay for the benefit that comes with it. The benefit is even more valuable, when we need to alter/fix somebody else’s tests or the corresponding code that is tested.
So should we write everything from scratch and limit the use of libraries to the minimum? By no means! We just need to be aware of how a given solution affects the readability of our code. There are so many gems and libs that make the coding of some aspects of our application easier. Many of the them encapsulate useful logic and solutions that would otherwise force us to reinvent the wheel in our project. But some are just adding the “magic” — let’s be cautious here. Metaprogramming is not a bad thing either, but let’s make sure we use it where it is necessary and more beneficial than alternative approaches. Finally, let’s keep our tests in a good, readable shape. If somebody finds it hard to understand the code we have written, the tests will be the place in which they will seek for the answers first.
One thing that is really painful for a reviewer when analysing the code is the same comment being repeated all over again; another remark about wrong indentation, yet another comment on using a better method alias, and still one more mention about a method not covered with specs… The good news is, if we get tired of repeating same comments all over again, all sorts of automation tools come to the rescue. Those that might indicate a healthy rails project can be divided into three categories.
Code style standardization tools and services will take care of the basics: how the code is laid out, if the code style is consistent (i.e. hound, rubocop), if best practices (i.e. Rails Best Practices) are applied when necessary or even if there are no obvious security holes (i.e. brakeman) in our implementation.
The second group is similar to the first in that it also depends on static analysis but it focuses more on such metrics as test coverage (i.e. simplecov) or the quality of code (i.e. rubycritic, CodeClimate, CodeBeat, PullReview).
The last group of basic automation tools and services encompasses CI / CD (continuous integration / continuous delivery) solutions. Even the most comprehensive test suite is not worth a dime if nobody runs it. CI services help in executing and monitoring the current status of our tests suite. It can even stop our deployment if we go red. There are many players on the market, i.e. CircleCI, TravisCI, Jenkins, CodeShip, etc.
Can we live without those tools and keep our project in a good shape? We surely can! It will however require much more attention, more thorough code reviews and a good amount of discipline. Why just not make use of this great services and be liberated to focus on much more interesting stuff?
A project that is driven by Outside-In testing, uses various design patterns to keep classes slim and testable, balances the code DRYness and readability as well as takes advantage of automated code analysis is most likely a healthy well-crafted piece of software. Even without looking at the code, if you inquire the team about those aspects of the development process you can get a general understanding of project condition. As regards new projects, this list may help you establish a set of practices that will keep the project bullet-proof from the very beginning.