What are some coding anti-patterns that can easily slip through code reviews?
Popular Programming Languages
What are some coding anti-patterns that can easily slip through code reviews?
Programmers are a notoriously irritable bunch. We’re constantly getting in arguments with each other about the best way to do things. This is largely because there is no one “right” way to code – it’s more of an art than a science. However, there are some coding practices that are universally agreed to be bad form. These are known as “coding anti-patterns,” and they can easily slip through code reviews if you’re not careful.
One common coding anti-pattern is “spaghetti code.” This is code that is so tangled and convoluted that it’s impossible to follow. It’s the software equivalent of a bowl of spaghetti – a jumbled mess that you can’t make heads or tails of. Spaghetti code can be very difficult to debug and maintain, so it’s best to avoid it if at all possible.
Another coding anti-pattern is “copy-and-paste programming.” This is when a programmer takes some existing code, copies it, and then modifies it slightly to suit their needs. This might seem like a quick and easy way to get the job done, but it often leads to duplicated code that is hard to keep track of. It also makes it more difficult to make global changes, since you have to remember to change every instance of the duplicated code. Copy-and-paste programming might be tempting, but it’s usually a bad idea in the long run.
These are just a few of the many coding anti-patterns that can easily slip through code reviews. So next time you’re doing a review, keep an eye out for them – and try not to let them slipping through!
What are the top 10 most insane myths about computer programmers?
Below is an aggregated list of some coding anti-patterns that can easily slip through code reviews.
Comments: We all want to write meaningful comments to explain our code, but what if someone writes 4 paragraphs of comments explaining exactly what a piece of code does? This will have no problem passing through the code review, but it creates frustration for developers who need to maintain the code because every time I need to change a piece of the code, well I have to go through the 4 paragraphs and maybe rewrite the whole thing, so screw it, I’m not touching that code.
SRP: We want out code to respect the Single Responsibility Principle, we want developers to write small unit of logic that can be easily testable, but what happens when you write too many units? This will have no problem passing the code review, and if someone asks, you can just tell them you wrote the code to be easily testable but then once you go over a certain threshold, it becomes frustrating to jump between 20 methods, in 10 classes just to do a simple task. It become the real spaghetti code.
SRP is a principle, not a pattern. From my experience, DRY should guide one to OCP and OCP to SRP.
Indifferent Architecture: You like a framework, so you use it for you next project and you don’t think much about it. You put all the Controllers in the Controllers folder, all the Services in the Service folder, all the Helpers in the Helpers folder and because frameworks (Rails, Laravel..) operates with a certain level of magic, the simple act of putting your Model in the Models folder will give you a certain level of assistance that you will love… This will have no problem slipping through the code review because guess what, you’re following the framework’s guidelines, but fast forward a few months and you end up with this monolith that we all like to hate and then your developers start hating on monoliths and want to go micro services… The real issue is not the monolith, the real issue was the lack of design and architecture.
The biggest anti-pattern that will slip through code reviews very easily is the singleton pattern. It is an anti-pattern for two reasons:
What is unique today may be duplicate tomorrow: the classic case here is that 20 years ago we used to have one screen per workstation, today two or even three and four screens are increasingly common. This means that if your development environment uses a singleton for the screen now you are in trouble!
Even if you really have just one (say, a configuration file), the implementations flying around are absolutely horrific 99.99% of the time
Right, so, why is the mainstream implementation horrific? Here is what people will generally do: because the pattern says that there must be only one instance of a class, they will hide the constructor and instead have a static method called “getInstance” or something similar to create the class and reuse it across the board.
That is the wrong way to go about it. What you should be doing is this instead:
Make the entire singleton class private
Have a normally allocatable class made public
In the public class’ implementation (which has to reside in the same file) create the private class as required (maybe as a static field! That is completely fine)
Use the public class
This is how you should do a singleton, but that is not what you see around. The net result of the common implementation is a hidden dependency on the singleton, which then means a lot of stuff cannot be tested properly without bringing the singleton in (so you can’t, for example, easily mock it).
Please stop doing singletons or, if you can’t, please do get them right.
Code reviews are really important. However, without a good set of coding standards, they can often become “this is my preference”.
Here’s my suggestion on how to avoid anti-patterns slipping through code reviews:
Read through Martin Fowler’s book “Refactoring”.
As a team, figure out what people think are anti-patterns.
Agree on a list. Define these anti-patterns in your coding standards.
Make sure everyone reads the coding standards, and can access it easily.
Then, you have given one another permission to call each other out when that class gets too large, or the method gets too long, or the method has too many parameters.
Bad Code:
Lots of comments
Meaningless names
long methods
methods that does many things
code that is hard to write unit test for
code that doesn’t have unit test
code that is tightly coupled to other code
code that isn’t S.O.L.I.D.
clever code
unreadable code.
Good Code:
Code that makes sense to another program or your future self in 6 months.
Statement, Methods and object has a single responsibility
coding pattern: Good code – bad code
The bad code has correct logic, but without comments leaves the reader guessing at the meaning.
The good code not only clearly documents what is being done, but gives a good idea of why.
Now how would one refactor these snippets to add a remote location bonus?
If you really want to know how to write good code check out “Clean Code: …” by Robert “Uncle Bob” Martin.
What’s the coolest coding pattern you’ve seen?
“Early exit” — the coolest and simplest thing.
Coolest Coding Pattern
The idea is to exit the code block as soon as you can. A few bonuses arise from this pattern:
Your code is likely more focused on the purpose of the block. Better at avoiding a kind of “run-on sentence” type of programming.
Reduced nesting. The same exact code can be written where the complicated code is within a nested bracket given a condition, but this helps keep your more complicated code at the tail end instead of nested near the top of a function.
Helpful to reinforce the fact that validation and parameter checking should be done first. You get used to it and functions start to look weird if they don’t validate input parameters.
Much easier for others to debug your code. Most of the validation is near the top. Less mental brainpower needed because the code is a bit more readable.
Personally, I really like how it makes my code look like block paragraphs. It makes it easy to skim and read quickly.
One function/file to rule them all. This is common in C/C++ for programmers who are still in the early stages of learning how to organize code. They will start filling either a single function (e.g. “main”) or at least a single file with their entire project’s code. This is not a bad way to start a project; I still do this myself. The problem comes when the programmer fails to realize that the code is becoming too large for the most basic organization strategy and keeps filling one container with all their code.
Too classy: Every single object gets its own class with constructors and methods for things which will never actually be needed. This is a textbook example of a programmer who has read a textbook on OOP but hasn’t been shown what good OOP code looks like.
The god object: There is one class with one instantiation which has its fingers in every single part of the program. It manages memory, maintains logs, synchronizes threads, and sends the manager his TPS reports for the day. Basically this is an OOP version of example 1 above, but is something you might still see in poorly maintained code.
Balkanization: The number of classes, files, and folders in your project is directly proportional to the number of developers, specifically because they do not cooperate on the same code and have balkanized the code base into a piece for each developer. This is a behavioral sink for software development in response to poor job security. What better way to secure your position in a company than for you to be the only person in the entire company who understands your code, and what better way to be the only person who understands your code than to be the only person who reads it?
OOP: Object orientation is almost always a stopgap measure to stop bad programmers from doing too much damage to a large code base. Given competent programmers, functional/procedural generic programming with lean data types is more scalable than OOP for the vast majority of projects. This is well illustrated by many C++ projects, where template programming is the actual backbone of the project with classes serving as a light layer of icing on the cake.
Fire and forget: How many times have you personally stumbled onto code that you yourself wrote not too long ago only to realize that you don’t understand how it works anymore? It happens to most programmers often enough that they resent having to edit old code. This can be remedied by explicitly writing down detailed documentation in the comments of your own code with the idea of communicating the actual purpose and design of your code for not just a stranger, but yourself in the future.
Review fewer than 200-400 lines of code (LOC) at a time: More then 400 LOC will demand more time, and will demoralise the reviewer who will know before hand that this task will take him an enormous amount of time.
Aim for an inspection rate of less than 300-500 LOC/hour: It is preferable to review less LOC but to look for situations such as bugs, possible security holes, possible optimisation failures and even possible design or architecture flaws.
Take enough time for a proper, slow review, but not more than 60-90 minutes: As it is a task that requires attention to detail, the ability to concentrate will drastically decrease the longer it takes the task to complete. From personal experience, after 60 minutes of effective code review, or you take a break (go for a coffee, get up from the chair and do some stretching, read an article, etc.), or you start being complacent with the code on sensitive matters such as security issues, optimisation, and scalability.
Authors should annotate source code before the review begins: It is important for the author to inform colleagues which files should be reviewed, preventing previously reviewed code from being validated again.
Establish quantifiable goals for code review and capture metrics so you can improve your processes: it is important that the management team has a way of quantifying whether the code review process is effective, such as accounting for the number of bugs reported by the client.
Checklists substantially improve results for both authors and reviewer: What to review? Without a list, each engineer can search for something in particular and leave forgotten other important points.
Verify that defects are actually fixed! It isn’t enough for a reviewer to indicate where the faults are or to suggest improvements. And it’s not a matter of trusting colleagues. It’s important to validate that, in fact, the changes where well implemented.
Managers must foster a good code review culture in which finding defects is viewedpositively. It is necessary to avoid the culture of “why you didn’t write it well in the first time?”. It’s important that zero bugs are found in production. The development and revision stage is where they are to be found. It is important to have room for an engineer to make a mistake. Only then can you learn something new.
Beware the “Big Brother” effect: Similar to point 8, but from the engineer’s perspective. It is important to be aware that the suggestions or bugs reported in code reviews are quantifiable. This data should serve the managers to see if the process is working or if an engineer is in particular difficulty. But should never be used for performance evaluations.
The Ego Effect: Do at least some code review, even if you don’t have time to review it all: Knowing that our code will be peer reviewed alerts us to be more cautious in what we write.
Lightweight-style code reviews are efficient, practical, and effective at finding bugs: It’s not necessary to enter in the procedure described by IBM 30 years ago, where 5-10 people would close themselves for periodic meetings with code impressions and scribble each line of code. Using tools like Git, you can participate in the code review process, write and associate comments with specific lines, discuss solutions through asynchronous messages with the author, etc.
During the last 6-7 years I’ve evaluated various code review tools, including:
Atlassian Crucible (SVN, CVS and Perforce)
Google Gerrit (for Git)
Facebook Phabricator Differential (Git, Hg, SVN)
SmartBear Code Collaborator (supports pretty much anything)
Bitbucket code comments
Github code comments
At some point I’ve also just manually reviewed patches which were e-mailed after each commit/push.
I’ve tried many variations of the code review process:
pre-commit vs. post-commit
collecting various metrics & continuously trying to optimize the process vs. keeping it as simple as possible
making code review required for every line vs. letting developers to decide what to review
using checklists vs. relying on developers’ experience-based intuition
Based on my experience with the code review process itself and the tools mentioned above, within the context of a small software company, I would make the following three points about code reviews:
Code reviews are very useful and should be conducted even in software which may not be very “mission critical”. The list of benefits is too long to discuss here in detail, but short version: supplementing testing/QA by ensuring quality and reducing rework, sharing knowledge about code, architecture and best practices, ensuring consistency, increasing “bus count”. It’s well worth the price of 10-20% of each developer’s time.
Code reviews shouldn’t require use of a complex tool (some of which require maintenance by their own) or a time-consuming process. Preferably, no external tool at all.
Code reviews should be natural part of development process of each and every feature.
Based on those points, I would recommend the following process & tools:
Use Bitbucket or Github for your source control
Use hgflow/gitflow (or similar) process for your product development
The author creates Pull Request for a feature branch when it’s ready for review. The author describes the Pull Request to the reviewer either in PR comments (with prose, diagrams etc) or directly face-to-face.
The reviewer reviews the Pull Request in Bitbucket/Github. A discussion can be had as Github/Bitbucket comments on PR level, on code level, face-to-face or combining all of those.
When the review is done, feature branch is merged in.
Every feature goes through the same process
So, my recommended tools are the same you should be using for your source code control:
Bitbucket Pull Requests
Github Pull Requests
Atlassian Stash Pull Requests (if you need to keep the code in-house)
What are some checks you always do on your code before you submit it for code review?
Unit tests are above the minimum threshold
Consistent naming convention with rest of codebase
No duplication of functionality
Properly linted/formatted code
Code Review Checklist :
Logic : Whether your logic is correct according to the use cases?
Performance : Check if there is a better approach/algorithm to solve the use case?
Testing : Whether unit tests [3]have been written? Do they cover all the scenarios and edge cases? Whether manual feature tests/ integration tests[4] have been performed? ( I usually omit the integration tests to be written at the time of code-review, I think it’s quite early. I am fine if the changes have been tested in a local stack )
SOR : I call this separation of responsibility. Is there necessary control abstraction[5] in your low level design? How modular is your codebase? Is there a DAO layer before the database? If there is a client layer? Is there a manager layer? How have you handled exceptions? Who is taking care of logging? How generic can their methods be? What kind of methods should they expose and what responsibility should they own at each level? Probably, this is the best place to inject your knowledge of Design Patterns[6]. Also, this component decides how generic[7], scalable[8] and extensible[9] your system can be.
Readability : Short and descriptive variable/method names. Strong use of standard verbiage without any grammatical mistakes. Method size kept small. Proper naming convention throughout the package be it camel case[10]or snake case[11]. Consistent naming of variables. Do not refer the same entity differently at different places in your code, avoid unnecessary confusion. Define scope[12] of every class/method/variable and make judgements of adding a new class/method thinking of who is going to use it? and who is not going to use it?
Automation : If there are few lines of code being written at multiple places, move them to a method/utility. Avoid redundancy. Make the best use of reusability[13].
Documentation : Draft the HLD/LLD over a wiki or a document. The key design decisions, the Proof-of-concepts[14], the reviews/suggestions by senior developers should always be consolidated at one single place. Although this point is not relevant for all the code-reviews but for the key implementation reviews, this serves as a recipe for the reviewer. Apart from these high level docs, make sure that you have javadocs/scaladocs[15] for all the public methods. Avoid comments as much as possible, make your code self explanatory.
Best Practices : Read the manuals/ articles/ research papers. ( very few scenarios ) of the frameworks consumed. Be an ardent visitor of Stack Overflow[16] and check for the best ways to implement a certain complex usecase and how the code abides by it.
I spend quite a bit of time reviewing code and some of the common problems I found are :-
Over architecture by creating lot of superficial interfaces
Premature optimization of code
Reinventing the wheel when something like this exists in open source or inside the codebase already.
coming up with a totally new pattern for doing things when such problem is already solved in code.
Trying hard to fit a design pattern into a code where its not needed (just because you read it few days back)
Very long variable names
Typos in variable names
No comments(I am ok with this if code is written like a book but sometimes you are writing something complex like an algorithm that wont make sense to someone newbie and leaving a one liner comment about your decision process would help people why you are doing it).
Lack of enough tests in new code.
No tests or borderline tests when mutating legacy code. Also no effort to make legacy code better.
Wrong technology choice
Introducing SPOF in architecture
Typical database schema issues
Missing indexes
Typos, using java conventions for db field names or mismatched conventions with existing field names
very long column names
Wrong datatype like strings for date or varchar(1) for boolean
Too bigger or too limited field lengths
Where can I have my code reviewed?
Since you’re looking to review your whole project, Stack Overflow , the Code Review Stack Exchange, and programming subreddits won’t work.
Here are some options that will help a non-technical person such as yourself:
Freelancers and Agencies
Consider hiring a more experienced freelancer or agency to review your outsourced team’s code. You might even be able to hire a local software developer to review their work.
Development Agencies – There are thousands of software development agencies around the world that offer code review. Similar to hiring freelancers, they start at around $10/hour. See this Quora question for tips for choosing a software development company. Be sure to read through the checklist for vetting and hiring them.
On-demand Code Review
If you want a professional option then look at PullRequest.com. It’s a platform for on-demand code review that works with GitHub, Bitbucket, or GitLab to provide code quality feedback from vetted reviewers. They can review your project for bugs, security issues, code maintainability, and code quality issues.
Etienne Noumen
Sports Lover, Linux guru, Engineer, Entrepreneur & Family Man.