What are some coding anti-patterns that can easily slip through code reviews?

Popular Programming Languages

You can translate the content of this page by selecting a language in the select box.

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.

    The acronyms are explained here SOLID principles (plus DRY, YAGNI, KISS and other YAA)

  • 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:

  1. 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!
  2. 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:

  1. Make the entire singleton class private
  2. Have a normally allocatable class made public
  3. 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)
  4. 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.

 

“Early exit” — the coolest and simplest thing.

Coolest Coding Pattern
Coolest Coding Pattern
 
 

The idea is to exit the code block as soon as you can. A few bonuses arise from this pattern:

  1. Your code is likely more focused on the purpose of the block. Better at avoiding a kind of “run-on sentence” type of programming.
  2. 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.
  3. 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.
  4. 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.

From a distance you can see how it forms blocky paragraphs.

SmartBear Software company published a small white-paper with 11 good practices for an effective code review process:

  1. 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.
  2. 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.
  3. 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.
  4. 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.
  5. 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.
  6. 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.
  7. 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.
  8. 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.
  9. 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.
  10. 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.
  11. 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.

Source: Quora

This is a bit longer answer to the question – tool recommendations are in the end.

First some background. I’ve written Master’s thesis about conducting efficient code reviews in small software companies, which was partly based on a case study which I made with our own projects in small (10 employees) software company producing apps for Mac and iOS.

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:

 

  1. 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.
  2. 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.
  3. 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:

  1. Use Bitbucket or Github for your source control
  2. Use hgflow/gitflow (or similar) process for your product development
  3. 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.
  4. 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.
  5. When the review is done, feature branch is merged in.
  6. 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)
  • 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 :

  1. Logic : Whether your logic is correct according to the use cases?
  2. Performance : Check if there is a better approach/algorithm to solve the use case?
  3. 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 )
  4. 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.
  5. 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?
  6. 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].
  7. 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.
  8. 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.

Footnotes

 
 

I spend quite a bit of time reviewing code and some of the common problems I found are :-

  1. Over architecture by creating lot of superficial interfaces
  2. Premature optimization of code
  3. Reinventing the wheel when something like this exists in open source or inside the codebase already.
  4. coming up with a totally new pattern for doing things when such problem is already solved in code.
  5. Trying hard to fit a design pattern into a code where its not needed (just because you read it few days back)
  6. Very long variable names
  7. Typos in variable names
  8. 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).
  9. Lack of enough tests in new code.
  10. No tests or borderline tests when mutating legacy code. Also no effort to make legacy code better.
  11. Wrong technology choice
  12. Introducing SPOF in architecture
  13. Typical database schema issues
    1. Missing indexes
    2. Typos, using java conventions for db field names or mismatched conventions with existing field names
    3. very long column names
    4. Wrong datatype like strings for date or varchar(1) for boolean
    5. Too bigger or too limited field lengths

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.

  • UpWorkFreelancerFiverrToptalCodementor, etc. – With rates for code review as cheap as $10/hour, there’s a range of quality.
  • 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.

error: Content is protected !!