Code reviews and naming conventions (or when the obsession for naming conventions misses the point of the review)

The Past

It has been well over 6 years since I had my code reviewed by my peers. As a development team leader I had reviewed the code of my developers, however most of the time they didn't review mine. This was due to either them being juniors or mostly uninterested. I had asked for reviewing and I always got a "A OK" answer. The organizations I worked for did not really enforce this either. They were busy releasing to customers and to damnation with technical debts (or tests but this is another story). TL;DR When in Rome do as Romans do

What to look for in a code review

My take on what to look for in a code review is quite simple.

Abide to the overall design agreed

Be more strict to juniors as they got a more detailed design hand-off. Be more lenient to reasonable changes by seniors or seasoned developers. Ask for the logic behind the change and maybe debate the change to a degree of common ground and understanding.

Find bugs 

Yes there may be tests and a developer may have been checking the code extensively. However we are bugs that create developers... 

Abide to coding conventions

This is a simple one right? Make sure variables, the camel case of methods or the grouping of methods by visibility abides to that 1000 line excel file someone created a few years ago. Make sure some naming patterns are kept, for example add the word Implementation at the end of a class implementing some interface.

Check for typing errors

"Elementary my dear Watson". If a variable name has a typo it will make it hard for the rest of the team to locate it.

General code correctness

Make sure proper design, best practices and coding patterns are used. Make sure anti-patterns are not. Clean technical debt. Remove  dead code and unnecessary comments or commented out code.

Names

Finally the reason we are gathered here today. There are 10 kinds of people when it comes to names. Those who spend 0-30 seconds on them and those who can spend days on them. I personaly never gave them much thought unless they were way off the beaten path or their meaning made absolute no sense.

The Present

Last summer I decided to make a change. Have fun again. See and design more systems, meet new people all the time but first and foremost learn a new thing every week if not every day. I joined a company of super professional and knowledgeable consultants, who also enjoy learning from one another all the time.
As such one meets all sorts of development methodologies. Some enforce religiously unit tests and code reviews (Good! Good! Emperor Palpatine, Return of the Jedi)

Names in code reviews

When designing a system with your team you may choose temporary names to call certain aspects of your system. Some times these names have synonyms and sometimes these names, however temporary, stick. If you decide to change the names, no matter what the reason may be, make sure to clearly communicate the change to your team members. If the change was not clearly communicated, when code reviewing just make simple request to make some name changing. Do not however put all or most of the weight of the review on names. Put that issue aside and concentrate on the more important things.
This portrays a trait me and some friends of mine have spotted in many team leaders in our industry. The need to stick to the minor meaningless details and (dis)miss all the rest.

Conclusion and insights

Stick to the important things first when code reviewing. If the names make an overall sense live with it. Teach your people how to code correctly. The names are subjective. Totally subjective. Enforcing the name  just enforces one's mindset on another. If this becomes a thing of habit down the road all the developer will remember is the naming OCD of the team leader. And the team leader will remember the developer as unable to comply.
And finally when being a consultant flow with the client. Live the sudden changes and quirks of each client and learn to be flexible between different clients.

Comments

Post a Comment

Keep it clean, professional and constructive. Everything else including ethnic or religious references will be removed.

Popular posts from this blog

NUnit Console Runner and NLog extension logging for your tests

Image
Have you experienced the frustration of tests running perfectly well on your local machine and then failing in CI? This all too familiar "It worked on my machine" comes to mind. You set off to investigate and after a while you realize you are too far into the rabbit hole between code and logs in Kibana and this investigation is getting you nowhere. Wouldn't it be nice if you could hijack the logger and print the logs in the test runner's console? TL;DR NLog can be extended easily and NUnit  Console Runner does not play well with simple console logs.   Photo by  Kaboompics .com  from  Pexels Enter Console logging In .Net in general it is not advisable to log to console in a multi-threaded environment, because the console is not thread safe. While logging simple strings may not cause any problems logging strings with interpolated variables may very well be sub optimal to say the least. Should you use any such debugging technique you are better off with a thread safe f
Read more

Tests code coverage in Visual Studio Code with C# and .Net Core

Image
While writing tests for you code goes without saying, there is much to be said about code coverage of tests. How much of your code is actually covered by these tens of tests you just wrote? If you work in a TDD approach most probably you don't have such "mundane" questions. Of course you code is 100% covered. You did make sure every test failed before you added that line in your code to make it pass... and that conditional statement... or did you? While purists would argue there is no chance of no coverage I beg to differ. Let alone if you write your tests after the code is written... After such and such years and experience in the industry when we are nearing deadlines some things are thrown out the window. They shouldn't but they are. TL;DR tools help with coverage. In this case .Net Core Test Explorer with coverlet and Coverage Gutters . Photo by  Startup Stock Photos  from  Pexels Coverlet to the rescue As I blogged a while ago I have made myself a warm
Read more

Applying an internal forums system in the company culture - thoughts

Image
Long gone are the days where, as children, we used to tie a string to two boxes and shout messages at each other. Communication for some of us has changed from phones with a round dial to smartphones in a decade. Having worked in several places with a mobile based solution for groups of teams and dev departments forum/chat system as well as  a couple of places with an internal forum/social media system in place, I have seen more often than not that the adoption rate of such a system is diverse. Some like it, some not and some participate on a whim. And in some places it is obligatory. However what happens when you find yourself accustomed to such a system and arrive at a place where there was none, and one is being applied. How does an organization make the shift from coffee machine pow wows to an internal forum/social/chat board. TL;DR Success of such a forum depends on... oh quite a few factors It is not the medium, it is the content This is not a post on the different solu
Read more