5

I've been assigned the task of reviewing my colleague's work. He claims it is complete and that he has done the checking.

When I reviewed a few pages (about 1/5th), I found some things (about 10) that can be improved. For example, "Month Year" should change to "Date" or other meaningful wording, some data may be incorrect, etc.

So I try to explain these points to my colleague, and ask him to make those changes. But in the middle of the discussion, he said it was too much to follow, he had to make those changes first before I talk about the next changes. He just turned his back on me.

Later, my supervisor called us to a meeting. At the end of the meeting, the supervisor asked me to write down all the changes, set the priority and email it to my colleague instead of telling him verbally.

I feel upset that I need to do extra work for a colleague to do his own job. Shouldn't the implementer make note of the review comments? Is it common for the reviewer to write down the review comments himself? Should I talk to my supervisor about this?

We are both senior staff, and I'm just a little bit higher in rank than my colleague.

  • 1
    Just to be clear: you reviewed his code, found about 10 things to improve in his code and started to explain them to him, from memory? Without any written notes at all? – Neo May 12 '17 at 9:06
  • I have my own notes, in very short form and symbol by myself – Prisoner May 12 '17 at 9:16
  • 2
    Think of it this way, by giving him a written list, while it will take time out of your own work, you will be helping the team. The next time he does work, perhaps he will go back through your list to make sure he doesn't make the same mistakes. Your fix list becomes a check list. – curt1893 May 12 '17 at 12:07
  • 4
    @curt1893 I wouldn't even say it takes time out of his work: it's his JOB as the reviewer. It's time spent on a directly assigned work task. – sleddog May 12 '17 at 18:24
  • 1
    Key phrase here: "...my supervisor ask me to..." — if you were tasked by your supervisor to do this, then it is your responsibility to do so. – heathenJesus May 16 '17 at 22:09
26

And at the end, my supervisor call a meeting with me and him. And at the end of meeting, my supervisor ask me to drop down all changes, set the priority and email to my colleague instead of verbally.

It's pretty typical to write down all the issues you find during a review, rather than trying to explain your points verbally. Without a written list, it's too easy for everyone (you included) to forget some of them, meaning that they might not be fixed. It also aids communication -- the author can look back on each point and come to you if any clarification is needed. Finally, it creates a paper trail: if your colleague claims that you never told him about some problem, you'll have a clear written record that shows that you did in fact find the problem and convey it.

But I'm just doubt or maybe feel upset that why I need extra work to do his own duty. So I would like to know is that just my own problem, or this is not a common and I should talk to my supervisor about this.

Writing down each problem that you find is part of the review process, and you should be doing it consistently. It really isn't much work compared to the effort of actually reviewing somebody else's work product.

There are many tools that can make it easier to review someone's work. Which one(s) you use will depend on the nature of the work and the tools that you already use. Some examples:

  • Github has features that make it easy to see and comment on the changes related to any pull request, so your notes are seen in context, and you only have to search through the code trying to figure out what changed.

  • Most office-related software (Microsoft Office, Google Docs, and Apple's Pages/Numbers/Keynote) let you attach comments to a document, which again puts your comment in context.

Listing your notes in some document or e-mail message separates them from the thing being reviewed, creates version issues (like "Do you have the latest copy of my review notes?"), and makes it hard to see the history of the review. If your job involves reviewing other people's work, or having your own work reviewed, you and your coworkers will save a lot of time and effort by using tools that are meant to facilitate the process.

  • Thanks for the explain. If that is typical, I will accept it. But how about "priority the items"? – Prisoner May 12 '17 at 3:30
  • 4
    I don't know what type of material you're working with (computer code, prose, contracts, a spreadsheet, etc.), so it's hard to say whether prioritization should be the job of the reviewer. For software, one would generally want to fix all the problems before returning it for another review, so priority doesn't really matter -- they're all important. Whether it's common for a reviewer to prioritize issues in your line of work or not, I think you should just see it as a sign that your supervisor trusts your judgement, so go ahead and do it. Is it such a large task that it's worth arguing about? – Caleb May 12 '17 at 3:42
  • Nope, not really. Just like what you said, we working on software, and I think all should be fixed before publish. So should I treat the "prioritize" to "In this phase" and "In next phase"? – Prisoner May 12 '17 at 3:53
  • 2
    I'd just arrange your list in order of severity according to whatever criteria you think are most important. That should lead to the quickest improvement in the quality of the work, and if the author has to be moved to some other project before all changes are complete, at least the most important things will be done. – Caleb May 12 '17 at 4:09
  • 2
    @Prisoner - You prioritize over what is critical to performance, and what is more "etiquette." Bad data or improperly processed information = critical. The project is essentially failing. If he titles a variable or field "MonthDay" instead of "Date" - but the app works, that's a lower priority. I know with many of my apps, something like "date" is also something that the systems or languages might use or reserve for system use, so there might be a reason for it. Definitely, writing down is part of the review process, not added busy work, IMO. – PoloHoleSet May 16 '17 at 19:28
17

In 17 years I've never been given a verbal code review- and if someone tried I'd ask him to email me the comments. I'm not going to remember comment #2 30 minutes later after fixing bug #1. Its the same reason we use bug tracking software.

You really ought to consider using code review software. It allows you to send reviews, track approvals, and comment by line, showing what lines the comment applies to. It also allows the reviewee to respond (sometimes the reviewee had reasons that something really is right).

Now if you have a particular issue that you think would be better explained face to face- that's ok. Have a discussion. Then document the result with the rest of the review.

As for you needing to do extra work- its a code review. As a "senior staff member" reviewing the work of other is part of your job. In a large team, it may be the majority of your job. And really it isn't extra work- in the long term you'll save time because these aren't going to come back as bugs later.

9

Your supervisor assigned you the task of reviewing a colleague's work, so doing that review is your work.

It is also entirely reasonable to send the review comments by email instead of verbally, as it only helps everyone involved to check if all the review comments have been resolved. Moreover, if either you or the colleague gets pulled into another task and/or this task is put on hold, it helps to avoid repeating the work the next time it is taken up.

I would advise against bringing this up to the supervisor, and certainly not using the "not my job" tone being used here, especially if you are a senior staff.

Since you ask, in a formal or semi-formal review, it is more or less standard for the reviewer to write the review comments, and the implementer to respond to the review comments, explaining how it has been resolved or why it cannot be resolved.

The only case I can think of where the implementer takes notes of review comments as the reviewer gives his opinion is when the "review" just happens by chance. For example, the reviewer happened to look at the code while working on his own task, and found something "interesting" that needs to change. In that case, the reviewer might just call and say, "Hey, I found this thing in this code that may need some rework, mind taking a look at it when you have some time?"

  • 1
    Thanks, I just need a clarify which you and Caleb did. As in my past experience, I never receive any written comment from my reviewers, that's why I feel uncommon for me. But if that is part of the workflow, then I should accept it and continue my work. – Prisoner May 12 '17 at 3:29
4

"I feel upset that I need to do extra work for a colleague to do his own job".

This seems to be a major lack of understanding on your part how software development works. There are several tasks that need to be done by different people to produce a new feature: Someone has to gather requirements. Someone has to turn this into a verifiable spec (which tells the developer what to do, and a checklist so that we can say the feature is "done"), then there is the developer writing the code, there is the reviewer reviewing the code, and the tester who will verify that the code works in real life. Each of these roles has to be done properly for success, which means for example that the requirements gatherer doesn't just gather requirements, but writes them down so the spec writer can do his job, the developer writes readable code to make the reviewers job easier, the reviewer delivers the needed information to the developer, the tester doesn't just test, but writes down what failures where found, and where to reproduce them.

As a reviewer, your job is to review code, and to give all the needed information to the developer in a usable way. Yes, writing down what you found is your job. You're saying that you are upset that you have to do your job; that's a very bad attitude to have.

Software development and review are usually done by the same persons. So if I had to give a performance review, I'd judge you by your ability to develop good code, but also by your ability to do reviews, both finding problems, and also communicating these problems to the person who needs to fix them. So right now, if you insist on doing this verbally, because you don't want to do work to support someone else, you'd rank quite low for me.

  • I agree with you if the job role can be clear. However, I'm the one gathering requirements, turn into spec (not verifiable yet, which not enough time), part of "developer", tester and also the reviewer. Anyway, this comment just try to express my feeling, not relevant to the case. And Thanks for your info, if that is my job duty, I will try my best to do it right. At this moment, I'm just try to do the best from my pass experience from my ex-supervisor. But it seems time to learn from other place. – Prisoner May 12 '17 at 9:23
3

I agree that you need to write things down, to avoid ambiguity and create a reliable record of your change requests. But that's only half the story. To give you the best chance of successful feedback on complex issues, you need to discuss them too. Here's why:

  1. Feedback works best as a two-way thing. (Often, that's not possible – eg with online feedback. But it is in this case.) He may misinterpret what you write – either technically or even taking feedback personally. Discussing it avoids that.

  2. If you rely solely on written feedback, you can end up tying yourself in knots trying to second-guess every possible interpretation of and reaction to what you're writing. Talking avoids that.

So a combination of the two can work well: create a list of review points, then go through them with him. That way, you get the best of both worlds.

Granted, this is more work. But taking shortcuts in communication usually ends up costing you more time in the end.

This reminds me of a blog post a colleague wrote on our website about giving written feedback.

http://writing-skills.com/five-signs-time-pick-phone

Items 4 and 5 are particularly relevant here.

  • Hi Rob, thanks for your answer. Are you linked in anyway to the website? Do you contribute to it? The SE network has strict rules about external links, any link with the website must be disclosed in the answer. I suggest you read How not to be a spammer – CalvT May 12 '17 at 17:21
  • Yes. (Hence mentioning it was written by a colleague – I wasn't trying to hide anything.) But I found this thread while doing some research just now. I posted in an attempt to be helpful, as it's something I know a lot about (having advised on this kind of thing for nearly 20 years). Sorry if it came across as spamming. The above link does support what I've written, and I figured my answer stood on its own too. So it seems to follow the rules. But please do correct me if I've misinterpreted them. Spamming is the last thing I'd want to be accused of! – Rob Ashton May 12 '17 at 17:30
  • In this case, no, you aren't spamming :) - your answer just needs to say it's your website, and I've suggested an edit to do this (you can accept it by going edit(1) at the bottom) – CalvT May 12 '17 at 17:31
  • Got it – thanks! (I'll edit it now and be more explicit from now on.) – Rob Ashton May 12 '17 at 17:32

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.