We’ve all seen it. We have all faced the horror of crap code. In this post I am going to give the basic pointers for avoiding having your code thrown back at you with a bright red FAIL mark all over it.

Clean code matters. Clean code is important and when code is submitted for review you are saying “Look I have solved problem X using Y and its amazing, look at how good my code is, isn’t it awesome”. Honestly this is your opportunity to prove yourself as an engineer / developer / programmer and to shout about your code and what you have achieved.

Code reviews are often perceived the wrong way and as a result many people fear them. You should never feel anxious submitting code for review but we all do. It’s human nature to do so. We all have that element of doubt about our own work. “is it really good enough, could I do something better?” or “What will Bob think about this, does it meet his standard”.

It’s worst when you are dealing with legacy code, then you get an even greater sense of being wrong but don’t worry about it, it is natural to feel that way. As long as you have done your due diligence and you’ve written the solution properly then you should be proud to put your solution forward.

What constitutes good quality code though?

Well there are several things you must bear in mind. These are the rules of coding and they must not be broken at any cost.

  1. Think the problem through This is the best piece of advice I can ever give you. A well thought out problem isn’t a problem it’s a solution. If the problem is too complex, reduce it to a problem you have solved before. Keep breaking it down until it becomes something recognisable. Only then can you move forward.
  2. Do not add complexity. Complexity is evil and bugs love complexity. The more complex your code, the more likely it is to contain bugs.
  3. Document Everything must be documented. Every method must have a docblock header stating what the method is for, what its parameters are and what its return value is. Every class should have a description explaining its use. If possible and if there is time to do so, include samples of how you envisage the class being used. This isn’t as important but believe me it helps people who come after you. This is especially as important when you’re writing new modules. If your API documentation isn’t up to scratch with real world examples, nobody (including you) will know how to use it in the future.
  4. Test first Write your unit tests up front before you even consider writing the solution. Writing your tests first help you pick the right paths to go down and help mitigate complexity. If you are working with legacy code where unit testing is not feasible, gather evidence of all the solutions you have tried, even the ones that didn’t work (explain why they didn’t work though). If you can’t add unit tests, cover your code with other tests. Functional testing is fantastic for this. Write Behat / Cucumber tests or add Selenium tests. If you’re doing this though, work every possible path through the code. Don’t just follow the happy path but focus on the unhappy paths as well. Following these will help you simplify things in the long run and save time when it comes to QA meaning your code can go out faster and you can forget about it and move on to the next thing.
  5. Comments Don’t overuse comments. Your code should be self documenting. Use intuitive variable and member names, these negate the need for commenting. Only add a comment if what you’re writing is complex and there is no other way of writing it. over-commenting makes code impossible to read. Things like $i = 0; // initialise $i at 0  are unnecessary and only serve to annoy other developers working on that code. Don’t do it. If you see comments like this in legacy code, remove them but be careful that you don’t remove something that is going to serve as a reminder in the future.
  6. Style Follow the same style as the code around where you are working. If it is new code you’re writing make sure it adheres to company or project standards. If you’re working with Legacy code which is written in a different style, write your code in the same style as the legacy code but clean it up and tidy up the code in the immediate area around where you are working. Think of this is like working in a boiler room. Before you can start working you first need to sweep the floor and tidy up a bit to ensure you can work safely. Get the rubbish out of your way and then you can see what you are working on more clearly.If the code around it is in hungarian notation and yours isn’t, it’s going to stick out like a sore thumb. Your code should blend in to its surroundings and become indiscernible from the rest. Think of it like going into jungle warfare in a bright red tracksuit. Do that and someone will end up getting shot (and it will most likely be you).
  7. Ask questions If you are unsure about something, ask. Don’t just blindly charge in or you’ll never achieve anything. I would rather you ask me a hundred pointless questions than make one mistake because you didn’t ask any.
  8. Refactor Cleaning up is one thing (this is usually a case of replacing spaces with tabs, re-formatting code to suit, etc.) Refactoring is another thing entirely. If you’re working with Legacy code, try and put it in a position where it can be broken out later and moved into something more suitable. Ask questions, is it still efficient or is there a better way? Is there time in the project to refactor that entire section? Will it break anything else if that is the case? Legacy code is often spidered in left right and centre so fixing it in one place may have adverse effects elsewhere.
  9. Automate Automate your testing and run the tests regularly. Don’t just run your unit tests though. Before you submit your review run checkstyle, codesniffers, linters and complexity analysers. Use tools such as Findbugs. These are tools designed to help you. Sonar is a great tool for doing just that so learn how to use it efficiently. It can take the results of automated testing and display them for you. Build your code in Jenkins / Bamboo / Travis etc. and have it upload the results to Sonar where you can see at a glance if the code you have updated as 100% code coverage or if you have introduced more complexity.
  10. Test Before you send the code out for review test it again. Run the unit tests and integration tests. If there are automated tests surrounding it, run them. Make sure your code works and doesn’t break anything. There is nothing more annoying than finding a change has broken a downstream dependency especially when your codebase is enormous and takes hours to build. If you’re in the middle of a release that change can cost real money especially if a time critical release gets delayed. Remember, once a train has started it cannot stop and you need to be onboard it, not underneath trying to derail it.

When you put forward your review you are stating a fact. “This is the very best I can achieve” but you’re also asking questions: “Is this the best way” and “Have I missed something”. If your code is well written, follows the company or project code style, is fully documented and there is evidence that you have tested it and run complexity analysers over the code first then your peers will praise you. Hand in something that has clearly been rushed, had little or no thought put into it and breaks any of the above rules then quite frankly you might as well get your coat now because it sure as hell isn’t going live.

Whenever I am carrying out a review of code I have 3 statuses I work with.

  1. Passed. This means that I am happy with the code I see, all the tests pass, it doesn’t add complexity (or if it does it is unavoidable and well documented) and it is a well structured piece of code.
  2. Passed Back. I’m not failing it but there are minor warnings and you need to clean it up. Perhaps you’ve used tabs instead of spaces or forgotten to document a parameter. These are trivial things, fix them and its good to go. I may also use this status if there is a simpler way of doing things, in which case I will offer a full explanation of why. If you want to disagree I’m cool with that but back up your argument with fact. I’ll accept it a lot easier if you do. I will give up to 3 passed backs before I fail a review depending on the size and complexity of the code. The less complex a piece is the fewer the chances to correct it. Remember, when I’m reviewing something (or you’re reviewing someone elses work), thats time out of my day so if I have to send it back more than once I’m going to get annoyed. You would do too.
  3. Fail. You better hope I never give this because this is reserved for atrocious code. If you get this from me, go away and rewrite the whole thing from scratch and don’t come back until it’s perfect.

Thats all for this post. In my next post I will show you how to set up your environment for writing clean PHP.

Leave a Comment

This site uses Akismet to reduce spam. Learn how your comment data is processed.