3

I am working in a university coding project of 8 people. We're working on gitlab and every time someone does a merge request, at least 3 people need to look over the code and upvote the request, if everything seems okay. If at least 2 people upvoted, the third will merge the branch with our main branch.

But whenever I see some mistakes, I don't know how to address my colleagues. I can text them privately or point it out in our group chat. But if I point it out in group chat, it seems like I am picky, but if I just text them privately, I fear that I would withhold information from everyone else.

Usually we communicate over a group chat on slack. We don't use the comments section on gitlab.

So my question is:

How can I point out a bug/error to my colleague without being the "bad guy"?

  • What happens if that merge request goes on for a month long with only 1 upvote? – Paul Karam Dec 3 '18 at 7:03
  • @Raditz_35 to give you a bit more information: This is a first for me to work with such a big group on something. So in my opinion I am being picky because it's like I am exposing someone to the group regards some bugs. Some are serious and some are minor ones, but nevertheless I always feel bad, if I want to point them out because I feel like there actually is a need for that. It actually also seems like noone else is pointing out anyone else's bugs etc., since my colleagues often just read the code and upvote. It doesn't seem like they even actually care about the details. – dnsiv Dec 3 '18 at 22:05
  • What have I done so far? As this is my first big project with other people, I am trying to restrict myself on the minor bugs for the groupchat and if something serious happens, I will text them privately. I always feel bad, if I need to text into the group or to them directly – dnsiv Dec 3 '18 at 22:05
  • @PaulKaram As we're doing weekly standups, a merge will at most just go on for some days – dnsiv Dec 3 '18 at 22:06
1
+100

Giving feedback well (and receiving it) is an important part of working in any team. Good communication is something you can work on. Giving good feedback is important for your colleagues or if you are a mentor or teacher in the future. Many teacher training programs (and HR) include strategies to do this. The literature is extensive on this topic so here are a few of the main strategies.

Sandwiching

Say something nice first. It’s a lot easier for them to take criticism if you meet them halfway. Say what they’ve done well or why their contribution is a step in the right direction. You won’t come across as so negative or excessively critical.

Thanks for your hard working this. I think it’ll be great when it works. We’re almost there. Here’s a few minor problems we need to touch up...

Give clear actionable feedback

Vague criticism is not helpful for anyone. If you see a problem, point out specifically what it is and why it needs to be addressed. If you can, suggest possible directions that could be taken to fix or improve it.

Code is a great example for this as you can point out exactly which lines are problematic. Then you won’t waste anyone’s time looking for the problem. Tools such as GitLab also allow you to edit each other’s code before you merge. It is expected that pull requests will be discussed and problems will be addressed before merging the code. You don’t submit a pull request when it’s ready to merge, you submit it when you’re ready to get help and feedback on it.

You’re a much more friendly reviewer if you offer to help, even in small ways. You should encourage others to submit their work for review early, rather than later. Getting feedback from the team is an important part of the project and giving harsh criticism will delay the project. If team members try to get things perfect on their own, it will take longer than if you troubleshoot issues together.

Give the Benefit of the Doubt

You might not know the whole situation so try not to assume why there’s a mistake. It could be an honest mistake. They could be out of their area of expertise. Whatever the reason, it doesn’t matter. You can ask if they had any problems they need help with or let them volunteer that information if they’re ready to. However, don’t assume that they’re out their depth just because they made a mistake. These happen to all of us.

Don’t dwell on why there’s an error. Point it out and move forward. Discuss how to solve it together.

Explain the error politely and respectfully. Don’t question their expertise or criticise them personally.

They likely have no idea that there’s an error in their code. They could well know how to fix it on their own if you draw attention to it. In this regard, if you do it kindly, you are helping them simply by drawing attention to a problem they didn’t know needed to be solved (these sorts of things are often discussed in the Issues of a GitHub repo).

Use it as a Teachable Moment

This is an opportunity to learn from each other. If you explain the problem clearly, they may be able to avoid making similar mistakes in the future because they know understand why what they did was problematic. If you ask them why they did it the way they did, you could also learn about their coding style and what they were attempting to do with it.

You can also suggest ways to improve their workflow to avoid problems like this in the future or so they can identify them themselves. You can suggest things like 1) commit more often so you can revert changes 2) track changes in a dedicated branch 3) write functions to avoid copy-paste or human errors 4) write unit tests to ensure functions are doing what you expect 5) document functions and comment code do others can understand what it does (and why).

There is a lot of literature on best practices for writing code. You can share such resources to help them improve. You can also discuss coding practices with your team as you may have different skills and experiences. If you can agree on shared conventions then it will be easier to work together. It’s also an opportunity to learn from others in the group who may have more experience with different tools.

Means of Communication

Forums such as GitLab and Slack are designed to give feedback to the entire team. I would encourage this. Many teams even do this completely public on GitHub. This lets the entire team know the situation and they can step in to help if they understand the problem better.

If you are worried that it’s a simple mistake, you can contact them directly and point it out so that they can address it without the rest of the team knowing. It depends a lot on the situation. If you do it respectfully, it shouldn’t be a problem for the whole team to know. We all make mistakes and a team with good communication will be understanding that it’s a work in progress. If your feedback it too cruel or unkind that you don’t want other members of the team to see it, you probably shouldn’t send it at all or think about how to give more constructive feedback.

I would recommend to give public feedback to the entire team unless there’s a good reason not to. Communicating privately should be the exception.

Be aware if power differences, even small ones. As a reviewer (or mentor), you’re in a position where your opinion is respected. They need your approval to merge their contribution. Be kind, patient, and understanding. Be aware that they may be shy to admit mistakes around someone they may perceive as more skilled and doesn’t make such mistakes. You can share some of your similar mistakes and experiences to show that you’ve done similar things to them before. Explain why you know it’s a mistake.

You should take another look at that part there. I did a similar thing and it lead to problems later on when we tried to do other things.

A good team member or mentor is someone you feel open to communicate with freely and whose advice and criticism is valued. As a reviewer, you should aspire to do this. It’s an opportunity to develop skills useful in your career.

7

The process of pull requests is meant for reviewing code and ensuring the master branch keeps functioning as expected. Buggy code shouldn't be merged, so it needs to be pointed out one way or the other.

You say you do not use the comment section that is provided by Gitlab. This is kind of weird. This makes it harder to comment about specific lines of code. Anyways, just use the accepted public channel. Make sure that all reviewers are aware of the bug so that it cannot be accidentally merged.

Priorities

You do need to figure out as a group how deep the reviews go. Is it to stamp out bugs, or do you also want to comment on formatting, style, patterns and so forth. It is important that you are on one line as a group. You don't want to be the only one commenting on all the style issues you might find, if the rest doesn't care about that.

Wording of feedback

Try to make sure to direct the feedback to the code, not the writer. "This code does this", "this line has a bug", instead of "you made an error here". This might prevent being seen as the "bad guy".

  • @JAD What about missing functionalities? For example, the code is doing what it should, but it's missing internationalization for its strings? This was something, that I was insecure, too. All strings in our .html templates needs to be in a certain language and loaded with certain libaries, so that those strings are recognized and get translated. So a teammate totally forgot all of that. My lecturer reminded about that, but at that moment, my group shove that priority aside, since they didn't value that much. I didn't want to text him about that, since this would look like I am nit picking.. – dnsiv Dec 3 '18 at 22:14
  • @dnsiv at risk of turning this into a Software Development question... You could do two things. One option is to put the internationalization on a backlog so that you don't forget to add that, and add it at a later point. This would allow your team to set priorities for different features (and requirements). The other option is to insist it is added now.A way to approach that is to point out that it is one of the requirements and that leaving it for now might cause it to be forgotten all together. – JAD Dec 4 '18 at 7:33
0

My go to in these cases is to pretend confusion and make statements by asking questions. Since I'm putting myself in an inferior position, I'm viewed as less of a threat to someone's ego.

For example, instead of stating:

There's a bug in this loop that can cause an index overflow.

I might say:

I'm sorry, I'm confused about this loop. Could this cause an index overflow or am I missing something here?

The advantage of this approach is that if you are in fact wrong or being nit-picky, you've also invited the team to comment on your gaffe in a friendly way.

-1

This is a university project which should prepare you for “real life”. In real life, you do use “comments”. You don’t do a majority decision, there is one or more reviewers and all must agree.

If you don’t want to accept a change, you add a comment saying why it is not acceptable. That’s it. The person reads the comment, changes the code, and puts it for review again, and everyone is happy.

The only “interpersonal” thing is that if you don’t accept code because of something that is your personal opinion, or because you are a jobsworth who delays progress and doesn’t actually improve the work, that doesn’t go down well and I’ve seen these things not ending up well for anyone involved.

And of course if you have a participant who feels attacked by this, then it’s that persons problem. Not just in your project, but long term in their professional life. If you get complaints when you failed a review for legitimate reasons then you can discuss that aspect with them carefully. In that case, ask another question.

Summary: Don’t worry. Code review isn’t supposed to be personal. If it becomes personal despite your efforts, that’s another question.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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