do you do code review at work?

ccanan

Newcomer
hi, I think the topic is not suitable for this forum, but I hope to get ideas from graphic guys here?
do you do code review from your work, if so, how?
here is my opinion:
for team consist of senior programmers, code review for every submit is not worth the effort.
as to fully understand the implementation and the context, it needs lots of time, and it's not so possible to find flaws.
but code review between each other every 2 month or longer is pretty ok, just like batching in d3d programming, it's worthy.

for team consist of less senior programmers and more junior programmers, code review is fine.
or when the project goes to the final stage, stability is extremely high
 
Your kind of missing the point of code reviews.
They do three things.
There is the obvious reviewer points out error, but it isn't the greatest part of the value.
When someone is explaining code to someone else, they read it differently than when they are writing it, and often spot errors in their own code, or edge cases they missed.
And more importantly IMO it increases general familiarity with the code base, stuff I code review I might otherwise never look at until I have to.

In my current job we don't do a lot of code reviews, I wish we did more, IMO they should be mandatory for any API change and people should ask for them for any significant functional change.

I've been write C/C++ code for over 30 years, most would consider me to be pretty good at it and I still make my share of stupid errors.
 
Yes. From my experience, It's useful to bring more junior folks up to speed and expectations. But it's not every check-in unless it's around API changes or critical paths (performance oriented review). Some teams I have worked with do sign-offs from another engineer, but it's really a glance and not a deep review.

And as ERP says, even experienced programmers make errors or miss potential bugs, races, under/overflow, etc.
 
We do code reviews of every single commit and I think it works OK.

It is a good way to decrease the number of issues (Ace coders make mistakes too) and reduce code rot, I'd say it is worth it. As ERP explained, getting familiar with the code and learning is another good point, if you review a piece of code you must really sit down and learn how that, and surrounding bits of code works.

Code reviews are done in Gerrit. It is a web based tool which gives a good interface for reviewing code.
 
We do code review for every commit as well. Prior to finding a tool that worked for us, I thought code review was terrible. If you do it via meetings or email, it's just too difficult and time consuming. It also requires the coder and reviewer to be together. If you use a tool, you can do it on your own time. The author submits the code and the reviewers review it when they have time. We use CodeCollaborator. There are other tools but when we reviewed them all, we found it had the best features and was easiest to use by far. We had to find one that works with multiple SCMs since we use multiple. It did the trick. So, I guess I would say, if you're doing code review with a tool, it's great. If not, well, it's much more of a chore...
 
...we have it (informally) for GIT pushes. I mostly find it redundant and pointless (but for enforcing code-style, yeah!).

However, when I switch to my real field and do ITSEC code review...

...panic!!!

...every company should do that.
Especially on valuable assets...
AND (imho) should not let it do to standard developers, but to real ITSEC guys.

That's my (long) experience on the subject.
 
We do code reviews of every single commit and I think it works OK.

It is a good way to decrease the number of issues (Ace coders make mistakes too) and reduce code rot, I'd say it is worth it. As ERP explained, getting familiar with the code and learning is another good point, if you review a piece of code you must really sit down and learn how that, and surrounding bits of code works.

Code reviews are done in Gerrit. It is a web based tool which gives a good interface for reviewing code.

Completely agree with this.
 
If you're reviewing never before seen code, it's definitely hard.

It's quite hard to do a code review on large chunks of code - so limit the size of changes between reviews. Branching and commiting often, into manageable feature sizes and changes really helps here.

We're using Git, Git Flow, and Atlassian Stash for our source management, which allows us to do code reviews on pull requests. Having a tool like this where you can analyse diffs between different branches and annotate/comment on changes in-line with the code makes the process really simple and easy. Another nice thing with this system is that the pull request can be declined, and you don't actually need to do the merge if you decide not to.

Previously I've done code reviews on every commit, which slows down your momentum, and makes you commit a lot less often (trying to make the code perfect before the commit/review), and we did this using diffs, which helps, but we had no special tools - just the diff tool. This style of code review wouldn't be my first choice - git with git flow is.
 
No code review for hardware (would be pointless)
Really? Why do you feel that's the case?

I've been write C/C++ code for over 30 years, most would consider me to be pretty good at it and I still make my share of stupid errors.
I'd say it's pretty much a constant factor for me. Perhaps if I monitored the bug rate more carefully I'd be able to predict when my code would be bug free :)
 
Really? Why do you feel that's the case?
Because the nature of HW is one of many things going on in parallel. You could do code review on major initial check-ins and look for some obvious booboos, but the majority of checkins after that will be relatively minor changes that fix a small corner case here or there that's hard to understand without looking at waves.

When I look at old diffs of my own checkins, I can usually derive what I was doing, but when I do the same for other people's checkins I'm usually completely stumped. Even if they document what they're doing, it's very often hard to match comments with the code. It doesn't help that everyone has their own coding style (and I'm not talking formatting here.) I've never seen coding guidelines for that: it's very hard to instruct people how they should write down their thinking about hardware.

Maybe I'm wrong here. Pointless is probably too strong. But I've never seen it done.

I'd say it's pretty much a constant factor for me. Perhaps if I monitored the bug rate more carefully I'd be able to predict when my code would be bug free :)
Now that's something I've seen in almost all companies I've worked for: tracking the bug rate on a weekly basis and making projections about when things will be done. Not necessarily useful on an individual basis but a good indicator of the overall stability of the project.
 
No, but we probably should. But, 90% of what I write is assembly so it makes it even harder. As long as it comes close to theoretical op counts and passes vectors from a C/Matlab model, we're good. We do have informal discussions about implementation techniques.
 
Maybe I'm wrong here. Pointless is probably too strong. But I've never seen it done.
Interesting, only because, in my - admittedly rather limited - experience, it was beneficial <shrug>.
Now that's something I've seen in almost all companies I've worked for: tracking the bug rate on a weekly basis and making projections about when things will be done. Not necessarily useful on an individual basis but a good indicator of the overall stability of the project.
Well I suppose because I'm nearly always doing "new" things there are nearly always new bugs :).
 
How do you do a code review on something that has no code ?
Digital logic is 99.9% described in RTL code.

The remaining 0.1% is schematic entry for digital logic inside analog blocks. And that's probably 0.001%, but I'm being generous.
 
There is a saying locks are for honest people. Similarly code review even at it's worst can avoid the temptation to check in something crap because of being in hurry or whatnot. We do code review for every commit(to mainline, branches get reviewed at merge) at work. Some reviews are tougher some are easier depending on what's being checked in. I consider codereviews to be very useful and essential part of development process. The exact way how to do codereviews depends a lot on the tools and processes.

OpenSource hasn't been mentioned but a lot of the succesfull projects have a form of codereview as part of the process...

And ofcourse there are exceptions. If one is doing some prototypes for non product use review process can be waived alltogether.
 
Last edited by a moderator:
Back
Top