How does your company do Code Reviews?

[CrazYmonkeY159]

So basically I am "new" to the industry and have worked at one of the big Software Companies where we did code reviews on some tasks/commits. This company was very mature and had long standing processes and procedures for this type of thing.

My question is for small to medium sized companies how do you integrate code reviews into the whole agile process? I ask because I am curious and well want to learn more about this topic.

 

[_kabal_]

Developers must factor in code review time and the resulting "fixes" into their schedule. They must schedule the reviews and ensure they get done.
Code review often and small pieces of work.
It's "peer review", not "I am the best, you are ****, let's make sure your code isn't **** too review"

As for the tools, we use bitbuckets pull request review. Will do this as a team. I will also sometimes just make comments on random commits

 

[zippy]

When a project/change/fix gets approved, developers, testers etc are allocated. Each CR gets a random dev on the project plan, other than the main developer allocated for a peer review before it is handed off to testers. Usually its a 4hour slot. Depends on the project. Could be more 4hour slots. Most never use the full 4hour slot. I have never. I think the project manager just accepts the template and multiplies the slots based on the dev time. I have a suspicion that its 1 slot per 40 hours

Tools: Brain and MK1 eyeball

 

[kripstoe]

Peer review. Rubber duck type of approach. People just talking to people what they've done, how the went about doing it and why. In my opinion this catches a lot of the major issues. Time is scheduled depending on how much needs to be reviewed. It need not be hours and hours. Tools: TFS.

 

[seanyp00h]

I don't want to totally hijack the thread but I think it is relevant here.

How are your developers performance measured? Where I work we have no way of measuring this. A manager would review a developer by giving a subjectivbe review - "Yeh I think Sean worked pretty well these past few months, missed out on a deadline or 2 but I think he deserves a raise".

This obviously isn't the right way to do it.

 

[Baxteen]

I don't want to totally hijack the thread but I think it is relevant here.

How are your developers performance measured? Where I work we have no way of measuring this. A manager would review a developer by giving a subjectivbe review - "Yeh I think Sean worked pretty well these past few months, missed out on a deadline or 2 but I think he deserves a raise".

This obviously isn't the right way to do it.

so in some situations it is the right way to go.

lets take the company I work for.
300 odd people, 250 or so in sandton, few in CPT, few in DBN, few in Brazil still, a few in England and a few in russia.

there are 4 developers, all of us in Sandton, 1 is a contractor managing systems he built and expanding on them.
the rest of us are split up and do vastly different things. my main focus is on workflow and reporting for the business unit.
the other person focuses on our Business to business system and the data management tools attached to that.
the other person focuses on the data management tools, and integrating our booking engine to suppliers.

so we all know and understand what we do, and how it fits in, but our challenges are vastly different.
I spend about 3 hours a day with consultants, understanding manual processeses to improve on them.

so me and the CIO really understand my project spec. sure everyone else can look at my code and understand it, but the point of it is lost on them. same when I look at their code.

reviewing the code is nothing.
reviewing the project once complete is the tricky part.
for the last 4 years I have gotten an increase, because my systems have saved the company loads of time and money. my one system saves 8 man hours per week, per person. so a team of 13 is now a team of 8, the 5 people moved to other positions.
I also save 2 reams of paper per day with the same project.
When I review that code, I cringe, because it looks terrible, but it has to be like that, there is no neat way of achieving the goal.
so if the review was based on the code alone, I should not get an increase.

 

[seanyp00h]

so in some situations it is the right way to go.

lets take the company I work for.
300 odd people, 250 or so in sandton, few in CPT, few in DBN, few in Brazil still, a few in England and a few in russia.

there are 4 developers, all of us in Sandton, 1 is a contractor managing systems he built and expanding on them.
the rest of us are split up and do vastly different things. my main focus is on workflow and reporting for the business unit.
the other person focuses on our Business to business system and the data management tools attached to that.
the other person focuses on the data management tools, and integrating our booking engine to suppliers.

so we all know and understand what we do, and how it fits in, but our challenges are vastly different.
I spend about 3 hours a day with consultants, understanding manual processeses to improve on them.

so me and the CIO really understand my project spec. sure everyone else can look at my code and understand it, but the point of it is lost on them. same when I look at their code.

reviewing the code is nothing.
reviewing the project once complete is the tricky part.
for the last 4 years I have gotten an increase, because my systems have saved the company loads of time and money. my one system saves 8 man hours per week, per person. so a team of 13 is now a team of 8, the 5 people moved to other positions.
I also save 2 reams of paper per day with the same project.
When I review that code, I cringe, because it looks terrible, but it has to be like that, there is no neat way of achieving the goal.
so if the review was based on the code alone, I should not get an increase.

That is our predicament. We all work on different systems so no developer can be measured the same. This is proving to be an issue now as somebody else is handling the reviews and they have nothing to base an increase on other then heresay.

This new person is wanting to implement some sort of KPI but i can't see how this will work at all.

 

[Baxteen]

That is our predicament. We all work on different systems so no developer can be measured the same. This is proving to be an issue now as somebody else is handling the reviews and they have nothing to base an increase on other then heresay.

This new person is wanting to implement some sort of KPI but i can't see how this will work at all.

it is a difficult situation.

what I would recommend is some sort of centralized project management system. defining a KPA can be tricky because you all have different goals in may than in august, at least that is how is works here.
if the projects are spec'd out, and planned properly, and you can keep track of progress though a project manager, then you could better do reviews.

I have been with my company for 5 years, my payslip says junior developer as my job title, but that is because I report directly to the CIO and getting around to changing it at HR is just too time consuming.
We did not get to this point over night, we have all spoken about it all and eventually came to this point about 3 years ago, it works.
a review should not be about code, or projects, it should be about what you added to the company in the last X months. if you can sit down with the person in question and talk about that, you are there, the rest is just paperwork

 

[seanyp00h]

it is a difficult situation.

what I would recommend is some sort of centralized project management system. defining a KPA can be tricky because you all have different goals in may than in august, at least that is how is works here.
if the projects are spec'd out, and planned properly, and you can keep track of progress though a project manager, then you could better do reviews.

I have been with my company for 5 years, my payslip says junior developer as my job title, but that is because I report directly to the CIO and getting around to changing it at HR is just too time consuming.
We did not get to this point over night, we have all spoken about it all and eventually came to this point about 3 years ago, it works.
a review should not be about code, or projects, it should be about what you added to the company in the last X months. if you can sit down with the person in question and talk about that, you are there, the rest is just paperwork

Thanks for this. This is what I've been wanting to suggest.

 

[Flojo]

so in some situations it is the right way to go.

lets take the company I work for.
300 odd people, 250 or so in sandton, few in CPT, few in DBN, few in Brazil still, a few in England and a few in russia.

there are 4 developers, all of us in Sandton, 1 is a contractor managing systems he built and expanding on them.
the rest of us are split up and do vastly different things. my main focus is on workflow and reporting for the business unit.
the other person focuses on our Business to business system and the data management tools attached to that.
the other person focuses on the data management tools, and integrating our booking engine to suppliers.

so we all know and understand what we do, and how it fits in, but our challenges are vastly different.
I spend about 3 hours a day with consultants, understanding manual processeses to improve on them.

so me and the CIO really understand my project spec. sure everyone else can look at my code and understand it, but the point of it is lost on them. same when I look at their code.

reviewing the code is nothing.
reviewing the project once complete is the tricky part.
for the last 4 years I have gotten an increase, because my systems have saved the company loads of time and money. my one system saves 8 man hours per week, per person. so a team of 13 is now a team of 8, the 5 people moved to other positions.
I also save 2 reams of paper per day with the same project.
When I review that code, I cringe, because it looks terrible, but it has to be like that, there is no neat way of achieving the goal.
so if the review was based on the code alone, I should not get an increase.

Your response reminded me of this : http://www.joelonsoftware.com/items/2009/09/23.html

 

[Baxteen]

Your response reminded me of this : http://www.joelonsoftware.com/items/2009/09/23.html

that just made my day.

I am not exactly a duct tape programmer. but then again, I don't care what it looks like as long as it works.

I am all for OOP and in line styling because that works in my head, sure the solution is not clean, and there may have been a better way to achieve that, but I thought of this way and did it like that.

 

[zippy]

I don't want to totally hijack the thread but I think it is relevant here.

How are your developers performance measured? Where I work we have no way of measuring this. A manager would review a developer by giving a subjectivbe review - "Yeh I think Sean worked pretty well these past few months, missed out on a deadline or 2 but I think he deserves a raise".

This obviously isn't the right way to do it.

We get measured on the faults(bugs) which occur after a change goes live. Also the seriousness of the fault. Also get measured on how many faults are raised in testing. How often we meet deadline etc. There are also project reviews where developers have an opportunity to explain faults etc. For example some projects suffer from scope creep which causes delays and this also results in a higher fault count in uat. These reviews help to ensure that the statistics are used correctly.

 

[rrh]

a review should not be about code, or projects, it should be about what you added to the company in the last X months. if you can sit down with the person in question and talk about that, you are there, the rest is just paperwork

You are confusing the two review types:

• A code review is - as it says - a review of the actual code written in order to assess the quality of the code, whether or not it is to specification etc. NB At no stage is the individual critiqued, other than (for example) to ask why a particular approach was adopted.
• A performance review can be performed at various times (e.g. annually, bi-annually, after the completion of a project etc.) and is used to assess the individual's performance over the period. Whilst a performance review is normally linked to salary increases, it should also include establishing the individual's weaknesses with a view to rectifying these e.g. via specific training.

A good KPI is objective; the best are easily measured, thereby eliminating the "human opinion" factor.

For example, one could use lines-of-code a.k.a. LOC.

So the KPA for a junior developer might be x LOC/day, whilst for a senior developer it might well be 2x LOC/day.

Similarly a junior developer might be expected to write code with no more than y bugs per 1,000 LOC, whilst for a senior developer it might well be half that.

 

[sp4ceman]

er as far as i know the better you are the less code you write and the longer it takes you to figure out just what those lines are

 

[semaphore]

One of ours will be test coverage, we currently have 75 an 65% coverage. The one test set takes about 30mins to run.

 

[Hamster]

We use Stash for pull requests/reviews. To me they are there to make sure there aren't any obvious problems and that you didn't miss anything like adding a deployment script without the corresponding rollback script.

We have one guy nitpicking at the moment rejecting pull requests because you spelled it "UserName" instead of "Username" etc. That to me is wrong - if everybody starts doing that we'll spend more time on reviews than actual work.

KPI? I dunno what these guys are doing but use contractors have 4 or 5 (and yourself) fill out those anonymous peer review forms. Works fairly well most of the time. Measuring KPI by lines of code and bugs per lines of code just sounds wrong and unreliable to me.

 

[rrh]

er as far as i know the better you are the less code you write and the longer it takes you to figure out just what those lines are

As I stated in my posting I used lines of code as an easy-to-understand example.

There are many such metrics, including that that take into account the complexity of the solution. The exact metrics used will vary from installation to installation, and should take into account the abilities of the staff.

It might well be that the more advanced programmers will write less code by using (for example) the more advanced features offered by the programming language. This generally results in denser, more opaque code that makes subsequent maintenance of said code a real PITA

 

[rrh]

We have one guy nitpicking at the moment rejecting pull requests because you spelled it "UserName" instead of "Username" etc. That to me is wrong - if everybody starts doing that we'll spend more time on reviews than actual work.

It depends on the language used: some (example C, C#) are case-sensitive.

Another problem is incorrectly-spelt variable names, where the original developer misspelt the variable, and you go looking for "UserName" only to find 30 minutes later - when you can't find any use of "UserName" - that he has spelt it "UserNam" ...

KPI? I dunno what these guys are doing but use contractors have 4 or 5 (and yourself) fill out those anonymous peer review forms. Works fairly well most of the time. Measuring KPI by lines of code and bugs per lines of code just sounds wrong and unreliable to me.

Wait until you have a boss who doesn't like the fact that you don't kiss ass and who uses your annual review to ensure that your increases - if any - are minimal.

Objectively-based KPAs have saved my ass on more that one occasion.

 

[Kilgore_Trout_Redux]

Story time :

I remember having formal continuous objective KPI based reviews.

I did incredibly well until my female boss took a severe dislike to me and tried to get me fired. (I effectively started doing hrr job better than she did and she got threatened.)

When called out by out CTO I pulled out a copy of all of my reviews and showed him.

About a week later I was called into his office with my boss who managed to force out a few tears and convince the CTO that I had effectively bullied her in my reviews to give me good marks. The CTO consoled her and effectively threw away the documented reviews.

Fortunately I argued to be moved to a different team and I was put on probation for three months.

Luckily I performed well and impressed my new boss who wasn't insane.

Moral of the story is even if you have a documented history of good KPI reviews you are screwed if your boss doesn't like you. Performance reviews are there to cover the companies arse.

 

[rrh]

I remember having formal continuous objective KPI based reviews.

I did incredibly well until my female boss took a severe dislike to me and tried to get me fired. (I effectively started doing hrr job better than she did and she got threatened.)

When called out by out CTO I pulled out a copy of all of my reviews and showed him.

About a week later I was called into his office with my boss who managed to force out a few tears and convince the CTO that I had effectively bullied her in my reviews to give me good marks. The CTO consoled her and effectively threw away the documented reviews.

Fortunately I argued to be moved to a different team and I was put on probation for three months.

Luckily I performed well and impressed my new boss who wasn't insane.

Moral of the story is even if you have a documented history of good KPI reviews you are screwed if your boss doesn't like you.

I share your pain, which is one of the reasons that I prefer KPI's that imbed measurable figures:

• Was the project completed on time ? Refer to the release documentation.
• Was the number of post-implementation bugs found within the KPI limits ? Refer to the bug tracking software.
• Etc.

Whilst it won't prevent management from ignoring the facts it certainly makes it more difficult.