Effective Code Reviews

[Thomas]

What steps do you do get good review comments from reviewers ? I am just curious to see how other engineers go about it ? When i submit code for review, i want other engineers to engage creatively about how the code can break not about "how their way of doing it is better than others" or a misplaced indentation in comments which has absolutely no effect on product. I eventually end up finding the possible flaws myself and review just seems to have no good effect at times.


--Thomas

[Combuster]

Companies keep standards. Not because the standard is the best, but because it makes things predictable and to allow anybody on the team to deal with your code as efficiently as anybody's. Don't confuse it with egocentrism.
Things like that are also much easier on people compared to mentally doing a full data flow analysis on your submitted code, which effectively means anybody can review your indentation, but not everybody has the mindset to effectively notice all the corner cases and antipatterns that might be present. And of course, there's quite often pressure on people to finish their thing, typically at the cost of your thing.

The one remaining legitimate issue here seems to be that bugs get missed in review. For that one, I hope you are skilled in the workplace art of discussing problems without becoming confrontational.

[SpyderTL]

If you are reviewing someone's code, just be clear about the nature of any issues that you discover. Minor issues, or personal preferences where you would have taken a different approach are fine, as long as everyone is clear that a) the code does not necessarily need to be changed, and b) you explain, briefly, why you would have done it a different way. (i.e. Aesthetics, performance, simplicity, readability, etc.)

On the flip side, if someone is reviewing your code, be sure you understand where the reviewer is coming from in regards to the items above. If necessary, ask specifically whether their issue is more of a personal preference, or if it is something that needs to be addressed.

Everyone involved just needs to realize that no two developers will ever agree on a particular approach, so unless you have made some sort of team agreement, or the company or client has mandated some sort of guidelines, all solutions should be considered acceptable as long as the end result meets the expected behavior.

It's hard to look at a block of code that is new or that has been changed by someone else, and spot logic errors in a 5 minute code review. Which is why we've moved to pair programming, or mob programming with up to 5 people in a conference room working on a single task. It is very unlikely that you will have a typo (= instead of ==), or a logic error (!= instead of ==), or forget a critical step when other people are watching. As soon as you deviate from their expected path, they will immediately start making noise. Another advantage is that once you are done, you will have 2-5 people who understand how the code works, rather than 1-2. However, this approach can be overkill for simple modifications, like label changes, or hiding/removing UI elements. But it does virtually eliminate the need for code reviews...

[Thomas]

Code: Select all

Companies keep standards. Not because the standard is the best, but because it makes things predictable and to allow anybody on the team to deal with your code as efficiently as anybody's. Don't confuse it with egocentrism.

Ideally this would be true. There are global coding standards. But an owner of a particular module might do it differently going against the existing standards and i fit my style to mimic it. My gripe is that comments should not be only about coding standards alone which are appreciated.

Code: Select all

there's quite often pressure on people to finish their thing, typically at the cost of your thing.

This is a real problem and to be fair it is not possible to catch everything in a review. However, I get the feeling that just looking at the coding standards is an easy way out!.

Most people are motivated and intelligent. I was thinking how we can tap into that without making it a lot of work.

--Thomas

[Thomas]

Which is why we've moved to pair programming, or mob programming with up to 5 people in a conference room working on a single task. It is very unlikely that you will have a typo (= instead of ==), or a logic error (!= instead of ==), or forget a critical step when other people are watching. As soon as you deviate from their expected path, they will immediately start making noise. Another advantage is that once you are done, you will have 2-5 people who understand how the code works, rather than 1-2. However, this approach can be overkill for simple modifications, like label changes, or hiding/removing UI elements. But it does virtually eliminate the need for code reviews...

Interesting reply. Thank you!.

[iansjack]

It's very long.

[Schol-R-LEA]

Pardon me, I need to go wash this blood off my face again...

[StudlyCaps]

Code: Select all

line_30710:
    II = ttt1 + 1
For JJ = II to III

Oh my

[SpectateSwamp]

The only person that can effectively code review this ---- Is Me.

You'd be a fool to think otherwise.
I know the shortcomings and the opportunities that this Fantastic app has.
AND

My review has nothing to do with loops or goto or anything else that doesn't matter.

I give this code a 97%

[iansjack]

Quite right.

Who else is going to waste their time reading thousands of lines of crap?

[StudlyCaps]

If anything that code makes a great case for review, because a reviewer wouldn't let you submit code that would be so difficult to maintain.

I don't want to discourage you SpectateSwamp, but if that is really your work, you should focus on learning some good programming practises. It really is quite poorly written.

[Schol-R-LEA]

I don't want to discourage you SpectateSwamp, but if that is really your work, you should focus on learning some good programming practises. It really is quite poorly written.

That's the problem. If you look at his posts on what.thedailywtf.com (NSFW, to say the least - not Swampy's posts, but almost everyone else's, including my own), you'll find that he's advocating - vociferously - for this approach to programming, and insists that 'noodling' is the only proper way to write code.

He feels that spaghetti code is easier to write, read, and understand than structured code, and insists that structured code is 'unreadable'. He also claims that all competent programmers really 'jam' their code the same way he does, and that advocates of other approaches are nothing more than a handful of 'perfect perfects' shouting down the ones who point this out.

A few relevant quotes from that thread and related ones:

Probably because I LOVE Line Numbers too. They tried to Steal line numbers from You but SSDS has brought them BACK. Every possible error must come from within this ONE module (how simple). I can just jam it and jam it till things fail.

Nested statements look like an eagle flying across the page and aren't that readable.
To be fair you'd need a copy of VB5 to jam it like I do. So you are somewhat handicapped.

Yup the "programmatically corrects" got rid of the line numbers but we can use pseudo line numbers.
When I want to make a change. I don't have to consider what other routines need to be considered. There aren't any.

Spaghetti code spawns serendipity not so with structured code. I can jam my code through trial and error and get results. Structured is not so flexible
90% of the greatest programmers coded noodle code.
I'm against anything done in the name of efficiency... unless it makes the code easier to understand.

It's not the code that counts it's the results.
The topic remains unchallenged...
There are a few examples in the code that the "perfect perfects" wouldn't like or may even drive them crazy
What about keep it simple don't you understand?
I have no problem diving back into code I wrote nearly 20 years ago.

Computers are thousands of times faster now...
Anybody that worries about performance is STUPID.

Here:

Maybe try Cobol or Fortran versions. There are probably more Cobol apps running businesses than Basic. They at least have GOTO's. How can you do any jamming it, without a simple GOTO to skip around some untested logic or past a display that you want to disable or view only when running in "test mode" Those overly nested systems have code looking like a bird flying across the screen. GoTo's clean that up. Local error trapping makes the code even more useless. I do most error checking using an "on_error routine" to simplify things and for testing. Good Example: Mid 80's. I had FlowCharted the billing, workorder and monthend ... logic for the CableSystem I was working at. Doing a 2 or 3 page chart for each. Not displaying the unimportant checks. Just those that were significant. After returning from a 2 month off-site conversion. I found that most of my charts were thrown out and the rest were in the process of being replaced with 10 to 12 pages for each function. I should have made copies. I should have made copies. It was easier to go directly to the program than look at those next to useless line for line flowcharts. That's the main reason for having all or most of the error checking moved down to an exception handling area for this Search. Far more understandable Far more easy to jam and test. This Search wasn't created by a Genius. So It doesn't need a bigger Genius to maintain or upgrade it. Much easier to test out 1 simple program that's Centrally trapped, Than a 100 or more adjunct parts that make it next to impossible to extend or upgrade. Better quit yapping and put in The jerky jerk backwards option. At the same time allow for a fast forward video playback. Watch out for those Systems that were written by a Genius. What are you going to do when your programmer gets old and sick. Or dies in a fiery crash. What are you going to do.
Yup if that language can't support this style of programming it ain't worth squat
We need a society to protect Cobol Fortran and Basic. No perfect perfects allowed.

Nobody shares computer knowledge better than this
Years ago a friend and mentor (Grant Cook) said that on the next system he developed all the data files would be TEXT only. Forget the integer, real and other numeric formats.
I'm a firm believer in that. When you need to do a data fix, how easy it is to use a simple text edit program like notepad.
I really love line numbers those that are against, don't have any idea how simple they make things. A magical noodle here and there and serendipitous events sometimes happen.
If they take away "goto's" I'm be sunk. Few of the purist programmers would have hired me for my coding methods. I wouldn't hire any of them either.

[Pre-emptive multitasking is a]nother stupid idea by Microsoft. Right up there after taking away line numbers.
Probably all to speed things up. You have to have slow code to make use of the computer power we have today. SSDS is poised to make good use of quantum computing.. Bring it on.

[iansjack]

Why are we even bothering with a guy who thinks that VB5 is the height of programming science?

Can't we all just ignore this guy until he gets fed up?

[StudlyCaps]

Oh wow, that's some powerful Dunning-Kruger energy.

e: Also agree with iansjack, he seems pretty happy with his "style" so probably not much point arguing the point.

[Solar]

You realize that literally all this user has posted so far was two trollish necro-thread-hijacks?

Don't feed the trolls.