Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have been doing a lot of code review lately, and I am unsure of the positive and negative effects and professionalism of putting positive and/or funny comments in code reviews.

We use Github as our code review platform on my team, so the comments are viewable by anyone. I generally try to use this platform so the entire process from start to finish is visible and historical.

share|improve this question
    
Yes, I realize this largely has to do with culture, but I am looking for a general answer. –  Pheonixblade9 Jul 26 '13 at 18:48
20  
It is important to reinforce what you want to see with positive comments. –  MichaelT Jul 26 '13 at 18:53
2  
I don't know if it's appropriate, but sometimes I post coding haikus in reviews: junior programmer - your uncommented code is - like splinters of pain. –  Yannis Rizos Jul 26 '13 at 20:46
    
I'm actually the junior programmer reviewing the senior's code - we have a pretty rigorous process here, all code must be reviewed :) –  Pheonixblade9 Jul 26 '13 at 21:33
add comment

5 Answers

up vote 46 down vote accepted

It's important to highlight positives as well as negatives. I know if I were reviewing the refactor of a particular hellish subsystem into something neat and clean, I'd probably buy the programmer a pizza for his efforts.

If you're using reviews as training, it's doubly important - highlighting a good piece of code will be helpful for the junior programmers also reviewing that code. They will have a chance to ask questions about why a particular approach or technique is better than another.

share|improve this answer
    
Can you add a note as to "why is it important"? I'll mark as accepted when you do that :) –  Pheonixblade9 Jul 26 '13 at 19:49
2  
It lets people know that what they're doing (writing good code) is appreciated, and keeps the focus on growth, which is what code reviews are all about. –  Jonathan Rich Jul 26 '13 at 20:51
1  
Can you add this to your answer? –  Pheonixblade9 Jul 26 '13 at 21:34
1  
+1 you deserve the nice answer badge for this one. The number of places that don't value constructive criticism enough always surprises me. Telling people they're doing a good job when they're doing a good job can be a surprisingly effective motivator. –  Benjamin Gruenbaum Jul 27 '13 at 14:48
add comment

Funny: Save it for the water cooler, except in minimal doses - having a prune face is not a requirement for reviewing code.

Positive: Certainly. Review includes both positive and negative/constructive, by defninition.

For the one receiving the thumbs up, you reinforce their confidence and inspire them to do more of the same through your positive feedback.

For the rest, as others have mentioned, they will learn what to do, as well as what not to do. They will also be encouraged to excel, so they too can one day be in the spotlight.

I once worked for a boss who was liberal in his positive feedback - the team was very successful and productive as a result. He moved on, and others took over that lacked his ability to praise a job well done. Productivity and morale took a nosedive, and many of the better team members left the firm.

share|improve this answer
add comment

I'd say keep the comments clean and to the point, exactly because of culture.

You cannot avoid that some people will take things the wrong way.
In order to mitigate that, a personal one to one talk will smooth things along, if face to face is not possible, chat or email or skype.

share|improve this answer
1  
Good point. Being "funny" in commenting can easily backfire, especially in multi-lingual, multi-cultural environments. –  David Navarre Jul 26 '13 at 19:46
add comment

Commenting in a code review is managing

Treat comments as a tool of management

Inserting comments into a code review is a form of management. As such, it should be approached as a tool of management.

Use managerial practices when commenting

There is a structure to managing people where the goal is to meet a desired outcome. Some of the main approaches to management will not apply in comments but most will. The applicable topics include environment, leadership, organizing, and controlling.

Environment

Culture

The environment dictates the style of management. The culture and environment of the workplace should be kept in mind when using any tools of management. Usually this is affected by the industry and the size of the company or entity being managed.

Style

If there is a lighthearted culture then that may come across in the style of management being used. If there are very strict guidelines, policies, and consequences then that should be reflected in the style used. So if everyone is on board for a star wars joke referencing droids and a weak minded stormtrooper then a comedic interjection may be applicable. However, if there are dire consequences of not taking the end result seriously then it may need to be avoided.

Leadership

Fundamentals

There are three main pillars of leadership to consider when commenting. Namely they are vision, communication, and judgement.

Vision

It is important when to keep the grand vision in mind when explaining or giving instructions. In comments, this could mean pointing out how small changes affect the project as a whole, what the implications are of taking different approaches, or a tip of the hat to separation of concerns.

Communication

Being a good communicator is important in many aspects of life. It is no different in comments. Employing a wise level of brevity is important - especially since comments shouldn't take much space. Get to the point early, and then back it up with an example if necessary. In a larger organization, this may also include the need to send out a communique or memo if the issue is not localized to one review session.

Judgement

It is important that strategy is used when judging whether or not comments need to be made, and what the changes need to be. Your judgement does not always need to be correct, but it does need to be consistently correct especially when large judgement calls are made.

Organizing

From a managerial standpoint, organizing will refer to keeping the end goal in mind and ensuring that processes are aligned to follow a set of rules. Comments should also keep this in mind in that the comments should when possible build on each other to make sure that the flow of design is followed. It will also be important to keep in mind the scope of the code being reviewed in order to reduce coupling and follow the overall design.

Controlling

Controlling the actions of those being managed is a delicate process. While being firm it should also be kept in mind that the people are important. There are several managerial skills to use while controlling others. These skills are political, conceptual, interpersonal, diagnostic, and technical.

Political

Politics can be found any time there is interaction between people. It is a huge topic, but in a general sense politics revolves around influence. It is important to keep in mind the personal and professional politics at work when making a comment. This could be related to an instruction, a joke, or even a question.

Conceptual

Managing through conceptualization is an important tool. It requires a complex analysis of the situation at hand. When commenting it can be beneficial to include some of the analysis used to come up with the conclusion or change indicated in the review.

Interpersonal

Interpersonal skills are very important when managing. This is also a large topic. Some of the important things to consider with interpersonal skills are mentoring, constructive criticism, and "harpooning".

Mentoring

It is important that management is seen more as a mentor than as an antagonist. In a code review, this means that sometimes it will be beneficial to include a nod to a design pattern or approach which could be used to improve a situation.

Constructive Criticism

Criticism is important because it invokes reflection. However, criticism should be kept as positive as possible when it can be. This means offering valid evidence to support the criticism, and also ensuring that the tone used is not negative. When reviewing code this could include showing an exception or possible scenario which would generate an error while implying a solution instead of showing every place which was incorrect when the whole piece of code would need to be replaced anyway.

"Harpooning"

"Harpooning" is when you figuratively harpoon someone to the ground. This is done by breaking them down step by step without any reprieve until the point that they feel unable to get up. If you harpoon a person in a code review or elsewhere you will lose their cooperation. It is important to avoid breaking someone down excessively.


Executive Summary

Treat comments in a code review as a managerial tool. Keep in mind that comments should be brief, to the point, and constructive. Also ensure that while commenting the person being reviewed is taken into consideration.

share|improve this answer
add comment

Code reviews are a tool for improving code quality, in part by uncovering defects. More importantly, you want to instill good coding practices.

From this perspective, it is important to comment on things done well. In a training context, improvements should also be commented on. If your culture is one of continuous improvement, then you should always be commenting on improvements.

Mistakes, bugs, and bad coding will happen. Point them out in a non-personal manner, and treat them as expected.

From a behavioral modification perspective, reward is much better at producing changes than punishment. I would consider having good work noticed as a reward.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.