Just my opinion, but Code reviews are a shit practice. In my previous company, we used to have one every Saturday. I presented my code and when everyone couldn't find anything to bitch/moan about, they said my code isn't "aesthetic" to look at. It's because I wrote this

if(condition)
do_thing;

instead of

if(condition) {
do_thing;
}

I cringe everytime I remember that incident.

Wait, public code reviews?

I’ve seen that at one other place and it was bloody awful. I don’t blame you!

@Root Sorry, but by everyone I meant everyone in the dev team.

@Sid2006 I assumed, yeah.

Generally code reviews are between you and one other dev, sometimes two, and either on GitHub or in person. Never in person in front of a large group.

The place i mentioned did that: we had weekly meetings where recent projects were put on the projector in front of the entire engineering dept. The author (or someone else) would explain the code, and everyone else would critique it. It was awful, and should never happen this way.

It's supposed to be the second frontier against bugs. (First being you testing your code first.) And a second set of eyes to see if it could be done in a faster or otherwise more suitable way. Not to forcefully get something to nitpick about.

@lankku But isn't that like not efficient at all ?

Person A spend 2 days working on something, write a bunch of code.

During that meeting, Person B says that it can be done faster using this and that...

So you dump all the work done by A in the trashcan only to implement the solution of B...

Aren't there analytic meetings before the actual coding ?

If person A finds his solution acceptable, why not keep it ?

@Grumm never had a code review meeting o.O' And it's not usually about fundamentally changing everything that's been done. It's more of 'you could use this function for more readable/faster/blaablaa' or 'you forgot to remove the commented out testing code'. And usually it's a fast rewrite as the tickets are max. 1d of work, right. Right? We're not talking about 'I wrote half a year this thing without anyone taking a second look'

They not only make you work on a Saturday but then also do meaningless bullshit on that day as well..?

@horus @LotsOfCaffeine Yes it was every Saturday 10 AM to 2 PM. Needless to say, I didnt stay there for long.

Another reason was they had me install HubStaff to track my keystrokes.

@horus haha yes, the salary was like 10% higher that others were offering which is what led me to start working there. The code review thing and Hubstaff thing was told to me later.

@lankku I am the only dev in the company xD

(Well more like the guy who make software, fixes the printer/pc, write BI cubes, make random websites, make new documents in InDesign/photoshop... 😥)

So never had a code review.

But I sometimes plan a few days to look at old code and refactor some stuff.

Curly braces are often preferred because leaving them out has lead into quite some critical bugs. Either way, it should be declared in the style guide and enforced through some other means than code review if so desired.

Code reviews are a piece of shit. You send an invitation to a bunch of people, nobody really reads the code, they don't leave comments before the meeting, and then they whine about things that they would've done differently. Like "why didn't you do it like that", well because I did it this way, "yeah well I like to do it that what", yeah well I do it this way, "yeah but still why'd you do it", I did it because it was ok in the last review, "yeah well now I changed my mind, I don't like like it".

And you have to invite like half a dozen people into the review. Most are guys whos code nobody has reviewed. Their own code is horseshit. But yet they show up to give their opinions on how things should be done.

Past the absurdity of this practice, if you have to break the body into a new line you should use braces. Single-expression conditionals exist for simple expressions.

How to break your code:

#define do_thing doA(); doB();

This is not good style but such macros exist. Always use brackets in languages like C, C++... If you want to keep LOC low, you can put { at the start of the line, then the body code and then the } still on the same line, like this:

if( foo(bar) < abcd(blabla,42) )

{ doStuff(144); }

Code reviews for especially dangerous or far-reaching things, okay, makes sense.

But code reviews as a matter of procedure? Bullshit.

I don't do that to my team. You are supposed to be trained, experienced, professional adults who have discipline and attention to detail and take responsibility for your work. All that being the case, I don't need someone looking over your shoulder for every little thing, least of all me as your lead. It's frankly childish and counterproductive in my experience.

Will mistakes be made sometimes? Of course, we're all humans. That's where responsibility comes in. Take responsibility for it, fix it ASAP, and everything is cool. But get the other things right and mistakes will be few and far between anyway (and good testing on top of that helps a lot).

Adding a new filter that'll hit every request? Okay, a review before push is a good idea. New to the team? Obviously yes, review everything for a while.

Otherwise, have at it. I trust my team.

Um, ”saturday”? I am so sorry!