4

As I understand, code review is to look for language/framework best practices, uniform conventions, guidelines and optimization paths. A while ago, I was peer reviewing code in scrum team and had some hot talk with my scrum master (She is QA too) as she insist me to look at COA (Criteria Of Acceptance) as well and check code functionality as per COA.

What I believe: In future, if someone looks at some block of code and feels WTF, then Code Reviewer is responsible more than implementer.

Am I blurred on the responsibility of code reviewer?

Improve this question
7
  • 1
    The "responsibility of code reviewer" is whatever your boss says it is. Are you suggesting that there's something wrong with looking over what a piece of code is supposed to do before making sure that it does that?
    – Sneftel
    Commented Apr 21, 2017 at 11:56
  • Well, I am confused about the specific responsibilities given to code reviewer in scrum world. Of course, Code reviewer should look on code seriously for things mentioned above, but between implementer and QA, this functionality test is not making sense to me.
    – Bishnu Rawal
    Commented Apr 21, 2017 at 12:09
  • 4
    Knowing exactly what code is supposed to do is an integral part of reviewing it. You're not just checking the indentation, you're also looking for subtle edge case bugs.
    – Sneftel
    Commented Apr 21, 2017 at 12:13
  • 2
    You should stop thinking in terms of "responsibility", at least in the sense of "who gets yelled at if something goes wrong". The reason people do code review is that it helps make code better, not that it shifts the blame.
    – Sneftel
    Commented Apr 21, 2017 at 12:13
  • Hmm.. If we stop thinking in terms of responsibility, then everybody in team should everything, is that feasible at all? I am not on negative edge like blaming/cursing of peers, i never do that instead I push them to learn things. But to me, it seems scope of code review is not upto functional testing since you have user story queue waiting for you.
    – Bishnu Rawal
    Commented Apr 21, 2017 at 12:20

3 Answers 3

Reset to default
5

Of-course, understanding the goal of an user-story before coding review gives a lot of context. In an Scrum team I would expect all team-members to already understand the acceptance criteria as everyone was part of the Sprint planning session.

Still actively verifying the (user/customer level) acceptance criteria of an user-story does not seem something I would pursue in a code-review sessions, as code-reviews are more about code-structure, readability, maintainability, knowledge-sharing, security and maybe even test-ability (I would check test code-coverage). Exceptions would be the cases of system level functionality where the whole story is technical.

I would argue that code-reviews is not a functional review. Although a good functional look at classes and code-responsibility might make sense, certainly in sensitive domains.

In future, if someone looks at some block of code and feels WTF, then Code Reviewer is responsible more than implementer.

In Agile (e.g. Scrum) you should hold the whole team responsible. If they notice technical-debt like this they should pay-up. Refactor the code and adapt their process to prevent it in the future. Blaming someone is useless.

Improve this answer
6
  • In my team testers often check what test cases devs covered before automating end to end tests. This is because some tests are better suited at lower level.
    – dzieciou
    Commented Apr 24, 2017 at 9:52
  • 1
    @dzieciou most tests are better suited at lower levels. As Martin Folwer puts it: If you get a failure in a high level test, not just do you have a bug in your functional code, you also have a missing or incorrect unit test. martinfowler.com/bliki/TestPyramid.html
    – Niels van Reijmersdal
    Commented Apr 24, 2017 at 9:58
  • 1
    I love UncleBob's work, certainly his books. A lot of the missings can also be found here: less.works/less/technical-excellence/index.html . I havent seen that video yet, thanks will look into it, seems it has some good comments.
    – Niels van Reijmersdal
    Commented Apr 24, 2017 at 10:25
  • 1
    @NielsvanReijmersdal Yes, I just mean that SDETs can participate in code reviews to decide whether enough unit tests has been written, check the coverage. There are some stories, especially technical ones that have acceptance criteria better verified at lower level rather than end-to-end tests. This is in oposition to what you wrote: "Still actively verifying the acceptance criteria of an user-story does not seem something I would pursue in a code-review sessions,"
    – dzieciou
    Commented Apr 24, 2017 at 11:34
  • 2
    @dzieciou Thanks. I agree and have updated the text to reflect the fact that there are user/customer focus criteria, but also system level criteria. In the end the real answer to the OP questions is "It depends". As it depends on domain, type of user-story, etc... Just use a good sense of logic and all will be well :)
    – Niels van Reijmersdal
    Commented Apr 24, 2017 at 11:46
2

As mentioned in comments, SCRUM is a process/methodology for executing functional work and not in the weeds on what "proper" work is/isn't. That being said there are certain checkpoints that need to be adhered to and I think you are touching on some of those, so this answer is scoped more to the touch points of reviews during SCRUM and not detailed specifics.

Each Backlog item should have acceptance criteria prior to being developed against. It should have also gone through a sprint planning activity. As the team is responsible for each piece of functionality collectively, then the following should happen for each piece of functionality:

  • Developed for release by end of sprint
  • Code Reviewed according to coding standards
  • Functionality reviewed/verified (code, testing, demonstration) according to description in backlog and acceptance criteria verification
  • Sprint Review of fully verified content.

With the above being said and the full team working it, someone should compare the developed code to the acceptance criteria in a code review and QA should test the coded functionality against the acceptance criteria to ensure it functions to spec. The Product Owner will also review in the sprint review to ensure they are happy with the fully implemented and verified functionality.

As far as "who does what" the SCRUM master should be aligning skill sets with task activities to best utilize each skill set in the team to the make the best team possible for producing the best product possible.

In short if the SCRUM master thinks you should be the one to do it, then there is probably a good reason for it. Discuss with your SCRUM master if you have any concerns or believe that a particular task item is better suited elsewhere.
Should someone compare acceptance criteria during a code review? Absolutely
Is that someone QA or another Dev or Sys Eng? Ask the SCRUM Master for assignments.

BTW - Reviewer makes comments and implementer should be responsible for fixing it...so not following the

What I believe: In future, if someone looks at some block of code and feels WTF, then Code Reviewer is responsible more than implementer.

comment. Especially seeing the implementation should be based on the backlog item with acceptance criteria just like the reviewer is basing the review on...

Improve this answer
8
  • 2
    Ask Scrum Master for assignments? Really? I though Scrum was a pull systems instead of a push system. Scrum Masters are NOT there to tell the team what and how todo it, unless its part of the Scrum-framework. The team should figure out when and who does what, not the Scrum Master. He/She should facilitate that the team figures it out.
    – Niels van Reijmersdal
    Commented Apr 24, 2017 at 9:22
  • Also I disagree on the fact that the implementer should fix it. Personally I would pair-fix it. Yes, you should discuss it, but anyone could fix it. Let's say the implementer is working on something else and another team member just finished a task. Why shouldn't he/she pick it up? Certainly in teams that are swarming this is so much better as it leads to shared code ownership, not you build it you own it.
    – Niels van Reijmersdal
    Commented Apr 24, 2017 at 9:50
  • You have some helpful content but not convinced. As I was watching uncle Bob, as he said scrum agile is marketed horribly with certifications and these creatures called scrum masters who has nothing to do with technical code quality (Which is ultimate truth of any product). Now they come up with "Software craftsmanship manifesto" to deal technicalities. I love the idea, already signed :)
    – Bishnu Rawal
    Commented Apr 24, 2017 at 9:51
  • 1
    @mutt The Scrum Master is not in charge of anything. He/she should defend and explain the process, not manage it. The team should share their tasks themselves. If only one person can do the work, he/she should pair and share knowledge so this impediment is removed, that is what the Scrum Master should do, namely coach self-organisation. But I think what a SM should and can be is out of scope of this question :) If you want to talk more about it lets move it to chat.
    – Niels van Reijmersdal
    Commented Apr 25, 2017 at 7:14
  • 1
    @BishnuRawal I wasn't talking about an attitude of the SM, I was referring to herding of tasks to appropriate resources to maintain velocity. Frankly I have seen many more individual developer attitudes that cause the failure of teamwork than a SM. I agree it's out of topic Niels van Reijmersdal, but still and interesting topic of conversation. I'm not sure how to spin off a chat from here, I've only done that when it prompted with a link before...
    – mutt
    Commented Apr 25, 2017 at 15:39
2

What I believe: In future, if someone looks at some block of code and feels WTF, then Code Reviewer is responsible more than implementer.

This belief is not SCRUM.

In SCRUM, the team is responsible, not the individual. If the code isn't delivered at the end of the sprint, or if it doesn't meet the acceptance criteria, or if it is of low quality, it is not the fault of any one person. If anyone is trying to assign blame, they are not doing SCRUM.

I realize that's not practical advice. The real world is a bit more complicated than SCRUM ideals.

Here's the practical advice: it is your job as a code reviewer to help the person who wrote the code you are reviewing. You aren't there to do a code review, you are there to help a team member. The code review is just the mechanism to make that happen.

If understanding the acceptance criteria helps you do that, than as a responsible member of the team you should understand the acceptance criteria. Not because the scrum master said you should, not because some guidelines say you should, but because that is the professional, responsible thing to do.

Improve this answer
1
  • appreciate your words, thanks. Real world software impediments, frictions and in-and-outs deviates to a great degree, making things debatable.
    – Bishnu Rawal
    Commented Apr 26, 2017 at 4:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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