Readit News logoReadit News
aequitas · 5 years ago
One thing I've learning is code review should never be used just as gate-keeping for the senior developers.

Rather, let everyone (juniors, QA, non-developers) in the team perform reviews of everyones code, even your senior code. Not only is it a great way to spread/communicate knowledge and understanding in the team. But everyone looks at your code with a different eye and background.

You may have written the greatest opus magnum of your programming career. But if the junior or medior can't make sense of it, they will not be able to maintain it when you're gone. So effectively you have failed.

capableweb · 5 years ago
> You may have written the greatest opus magnum of your programming career. But if the junior or medior can't make sense of it, they will not be able to maintain it when you're gone. So effectively you have failed.

Overall I agree with you, yeah. But also no. Of course the code should be as simple to understand as possible, but if the junior or medior doesn't understand matrices, nothing I do will make them understand them.

So I mean, yes, but also no. Sometimes it's OK to expect people to have some domain knowledge. Doesn't mean your code should be spaghetti of course.

lhorie · 5 years ago
> if the junior or medior doesn't understand matrices, nothing I do will make them understand them

Oh but there are things we can do. We can add comments that link to resources, we can write long prose (both inline in comments and in out-of-band documents such as RFCs and documentation) explaining why we're using something that nobody else in the team understands vs the alternatives, and we can level people up via informal sessions like lunch-and-learns, info sharing sessions, etc.

It's true that many juniors aren't capable of following concepts above a certain level of difficulty, but it's also not rare for people to really flourish if given a gentle push.

There's definitely a paretto balance when it comes to knowledge sharing. Don't be that person that writes zero docs and hogs all the knowledge when you could spend 20% effort to level up 80% of people to the point where they can start helping each other.

Kalium · 5 years ago
> Overall I agree with you, yeah. But also no. Of course the code should be as simple to understand as possible, but if the junior or medior doesn't understand matrices, nothing I do will make them understand them.

There is a difference between code that is complex because it needs to be (ex: matrix mathematics) and code that is complex because you wanted it to be (ex: microservices and kafka and obscure functional languages for what could have been a Rails CRUD webapp).

efsavage · 5 years ago
> if the junior or medior doesn't understand matrices, nothing I do will make them understand them.

Teaching them will.

aequitas · 5 years ago
Maybe they don't have to understand the detailed inner workings of an algorithm and such. But they should be able to deal with that piece of code in abstract. A good comment on what a piece of code is intended to do is perfectly fine to know it's place in a greater whole. Not every developer knows how crypto functions work, but they can make good decisions on how these functions should fit in certain logical scenarios or if an implementation is badly written/documented.
ramy_d · 5 years ago
yeah I would say in principle this makes sense but in practice a junior will typically say "this is beyond me but you're the senior so if it works, it works!"

It is an opportunity to go through the code and concepts at least.

8note · 5 years ago
Do you expect these developers to be able to debug this code after its in production? Or is that bottlenecked to yourself?

Having most of your team not be able to understand the code they support sounds dangerous

zffr · 5 years ago
> Rather, let everyone (juniors, QA, non-developers) in the team perform reviews of everyones code, even your senior code.

I think the OP is just saying that juniors should be allowed to participate in the reviews of senior engineer code (not that a junior review alone is sufficient for merging a PR).

I think this is a great way for juniors to get exposure to the complex topics they might not yet understand, but should learn more about.

wonder_er · 5 years ago
> but if the junior or medior doesn't understand matrices, nothing I do will make them understand them.

What if you took a hour or a day to help educate an eager junior developer about what you did in that PR?

I, personally, don't know matrices. I can damn well learn them if it becomes relevant to me.

If we were on the same team, and I said "what's this stuff?" and you said "nothing I can do will make you understand this" I'd be pissed, and that attitude would _permanently sour_ the relationship.

I imagine there was a point in time that _you_ did not understand matrices. I've no doubt that matrices can be tricky, difficult to understand, etc.

So, lets work together on sharing that knowledge! Once you've helped me learn it (pointing me to resources, pairing with me on an example problem solving session involving matrices) I will be able to maintain the code, understand it, and apply the pattern to novel problems.

But not just that, because I am a compulsive documenter of what I've learned, as you teach me, you help me teach _everyone who comes behind me_

TL;DR the attitude of "if jr/med devs don't already understand matrices, nothing I do will make them understand them." is _extremely_ self-limiting, locking you out of tons of potential value, and causing your entire team to be worse off.

It's not your fault, per se. I know this attitude is common in the industry. It just really pisses me off, because so many teams are deeply damaged by it.

(I know, I know. Maybe you'd love to slow down and teach early-career devs, but the feature death-slog required by management precludes you taking that time... in that case this isn't directly your fault, etc.)

Mauricebranagh · 5 years ago
Shurly the point is you a document it and if required walk them though the code and mentor them.

And not to what some one did mumble years ago when I was taking over the map part of a map reduce product built in PL1/G and say after 3 days "haven't you learnt it all yet"

hyperpallium2 · 5 years ago
Will it be maintained, and whose fault that is, are separate issues.
vbtemp · 5 years ago
I enthusiastically believed this in the past, but then I started observing the reverse: Code reviews became gatekeeping for junior developers... In other words, junior developers were using it as opportunities to assert power (via obstruction). Of course this was idiosyncratic to the company and team.

I've become less positive toward code reviews over the years as well - the low-hanging fruit of style checking, linting, and related stuff has been automated away. Moreover, people are busy and when they do code reviews, they are mostly looking for easy stuff to pick at. What ends up getting avoided is the far more important algorithm, design and architecture reviews that really require a much bigger cognitive load.

rightbyte · 5 years ago
I feel code review is overrated. It makes refactoring and small fixes take like 2h longer. I atleast fill up my parallel tasking slots too fast with small things if they linger in review. So either I don't do them or get less done.

Code review might be good for a subset of code, like retrofitting bugfixes to release branches or babysitting a team of beginners.

gwbas1c · 5 years ago
Code reviews turn into a formality when everyone is submitting great code. This is a good thing.

BUT: I just joined a team with domain experts who do not have a software engineering background. As I go through the code, there's lots of copy & paste, incorrect error handling, inconsistent (and confusing) variable names...

These developers really can benefit from coaching in a review. The team then benefits because the code is easier to read, robust, and maintainable.

(The codebase is impressive for people who have minimal software background. I've seen much worse from people who should know better.)

spaetzleesser · 5 years ago
“Moreover, people are busy and when they do code reviews, they are mostly looking for easy stuff to pick at. L

That’s a real problem. Almost nobody has the time to really understand the code they are reading and how it fits in.

bowlich · 5 years ago
Conversely if you do attempt to do a deeper dive into the design and architecture choices my experience has been you end up getting push back and overruled -- because people are busy and don't want to miss a deadline with code that "works"/ passes whatever the minimum bar was set to hurl it into production.

Deleted Comment

nitrogen · 5 years ago
As an opportunity to learn how to read and maintain the code that is being written, and to spot random mistakes that slip past the linter that everyone makes once in a while, yes. In that sense everyone should participate in code review.

But if you are giving veto power over code and designs to your most inexperienced developers, you will not have a good time.

Gh0stRAT · 5 years ago
Early in my career, I learned so much from reviewing other more senior peoples' code.

I suppose I technically did have "veto power" as my approval was required in order to merge the PR, but nobody ever thought of it in such adversarial terms. We'd comment with questions or things that were unclear, occasionally asking "why are we doing it this way instead of that way?" and more often than not the solution was simply adding/updating comments to make things more clear.

For our team at least, it was a casual and collaborative effort where everyone learned from each other. I wouldn't hesitate to give inexperienced devs veto power over my PRs again unless it was for a time-sensitive hot-fix.

aequitas · 5 years ago
Mileage may vary and it depends on the direct impact an approved review has (ie: is it CD to production or are there other quality gatekeepers), but giving junior members this responsibility can greatly help them in personal growth. They can always ask for a co-review if they think they are not able to make a decision, which itself can be another learning moment. And you don't have to tell them you secretly also review the code before it is pushed to prod ;).
Nursie · 5 years ago
> even your senior code.

Especially your senior code, by the time you get to being an experienced engineer you really should appreciate the value of lots of eyes on your work.

You may indeed have written the greatest opus magnum of your programming career, but I bet there are a few little bugs or edge cases or something you've overlooked. There's always something.

eithed · 5 years ago
I strongly disagree - while juniors are more than welcome to look at the PRs, if you allow juniors to approve PRs of other juniors you quickly find your codebase deteriorating.
apengwin · 5 years ago
That's not what they're saying. Srs. should still review jr. PRs. They're arguing that Jrs should also review Sr. PRs
taylodl · 5 years ago
I've found code reviews to be a waste of time. Here's my primary concern: what evidence do you have that this code actually works? I want you to show me your test plan, I want you to show me you thought through how this code is going to be used and have tests for it. I'm way more interested in that than I am whether variable names use camel case or snake case or whatever. In fact if your tests are complete, and pass, then we can always refactor the code later if we need to.
jaxwerk · 5 years ago
> Here's my primary concern: what evidence do you have that this code actually works?

This is a strange concern to try to address with code-review. I don't think most folks actually expect to catch a lot of bugs by staring at diffs. Testing is the right tool for that job.

> I'm way more interested in that than I am whether variable names use camel case or snake case or whatever.

Style concerns definitely shouldn't be the primary focus of a code review. Ensuring some stylistic consistency through the code base is a benefit they can provide, but certainly not the the most major.

> In fact if your tests are complete

How do you know if the tests are complete on someone's work before they merge it? Don't you need to get in there, and well, review their code to see what tests they wrote? Evaluation of test coverage by a peer is one of the more important parts of a good code review. What other process would you use to ensure incoming patches have the test coverage you want them to have?

> we can always refactor the code later if we need to.

Isn't it better to catch code structure issues when they are fresh in everyone's mind rather than taking on technical debt and addressing it down the road when it will require more effort? We've all written code in somewhat silly ways. My experience is that when a peer calls that out before I merge it, it's way easier to fix up-front than a year down the road when I'm in there trying to fix some bug and am wondering if I should just refactor the mess while I'm at it.

aequitas · 5 years ago
Bingo. If your function is properly tested or your _intent_ is properly documented/commented I don't care how the code looks (to a degree), as long as I have all the information I need to refactor it right there.

Also reviewers shouldn't squabble about syntax or formatting, that what we got computers for to do for us using auto formatting tools. Though it does help if a team converges on a similar style for which reviews are great as you quickly learn each others style and cross contaminate them with your own.

dm03514 · 5 years ago
I have the same mentality. I feel like almost everything is an implementation detail. Every person's implementation is likely to be different. In my experiences, even the poorest code is maleable (i.e. software :p).

PR's become more of a gate to see if you understand the problem and test for the solution opposed to HOW you went about it. Maybe I've had bad experiences but focusing on HOW is really prone to toxicity. I try to avoid that in PRs at all cost and foster a culture of collaboration, shared understanding of the PROBLEM and Respect for the Constraints. I've left places because of extremely toxic PR culture, so I'm probably pretty jaded :p (The culture was so bad I dreaded asking for reviews.)

How did you prove that your solution works and respects the constraints?? How do you protect yourself and others from future changes? Everything else is an implementation detail.

spaetzleesser · 5 years ago
I often find stuff and also get suggestions for things that could have been done better. I view code reviews more as a mutual learning experience similar to pair programming. Checking for variable names is silly.
dougmwne · 5 years ago
In QA I have found a code review to be very helpful in identifying test cases. Often I'll think I've covered every edge case I can think of, but once I've read through the code I identify the implementation can fail in a certin scenario I hadn't considered. This is why I think Black Box testing is a bad approach unless something about the circumstance specifically calls for it. I do think it's better to have a test plan first, then supplement it with code review, just to not get overly biased.
mathgladiator · 5 years ago
This all depends on your dev environment; a code review should also have tests with the diff as well as test results, and the more automated the better.
thinkingkong · 5 years ago
You need to redefine success if you think that’s failure. The software is almost by definition not a magnum opus if it isnt understandable or well documented. This attitude is somewhat pervasive but we really need to set better examples through clarity; not cleverness.
hinkley · 5 years ago
Everyone is the hero in their own story.
_throwawayyyyyy · 5 years ago
exactly - was on a team where an individual contributor got promoted vertically to manager and thought they could create a good product by code reviewing everyone to death and demanding that they be the only one allowed to code review. no more tagging other teammates; just this manager
legerdemain · 5 years ago
A person with the job title "senior developer" has, what, 3-5 years of full-time work experience? So instead of being 21 and bushy-tailed, they're 24-26 and only slightly less bushy-tailed.

They're still working through the maladies of youth, like absolute opinions, rabbit-holing, perfectionism, zealotry, and over-identifying with their work. A young person might need 10 years or more to develop a balance of caring and DGAF skills. And some people stay insufferable forever!

NotPavlovsDog · 5 years ago
I would like to take the parent topic a step forward and add "Mindsets to pursue as a Software Developer, regardless of rank". To add:

   What are my blind-spots? Actively seek and solicit contrarian viewpoints, contradictory to ingrained paradigms. Look for unquestioned self-beliefs, with dedicated time plan and check-lists.
As a personal example, the Lisp legacy has made me reconsider not just memory management, but default behavior when a program crashes, system design and, ultimately, the interplay between mathematics, CS and UX.

As a follow-up, the legacy of Douglas Engelbart. He demonstrated the computer mouse, hypertext, networked computers and face-to face conferencing, including shared workflow, in 1968 ![ https://en.wikipedia.org/wiki/Douglas_Engelbart ]. This was not mentioned in popular resources nor courses I took. I have learned a lot from this case, such as what being a true visionary vs marketer means and entails. I would have missed this entirely had I not allocated time and dedicated schedule towards a mindset of actively seeking contrarian view-points and historical perspectives.

  Dedicate time to self-review, including value alignment and career goals, according to 1 year, 3 year, and 5 year plan(s). 
The bigger picture, such as taking the time to review if the software one is producing aligns with own ethical values, and whether it meets own standards of engineering excellence, can often be muted by the mundane, the daily, and the profane/corporate.

iio7 · 5 years ago
I wish we would stop comparing young vs old, this vs that and then just help each other and respect each other. Everyone has something to contribute, whether it is experience, energy, knowledge, etc.

In my younger years I worked on a project with a much older guy and he was extremely meticulous to the point where it was really really difficult to stay patient. However, I learned that he did that out of several decades of experience, and that getting the stuff right from the beginning saved us tons of hours of work later.

ivansavz · 5 years ago
I'd like to add a fourth mindset that I have found to be very useful: try to keep the beginner mindset, even as you become more and more experienced.

As a (motivated) beginner, you're expect to fail, and fail repeatedly, and such setbacks do not affect you emotionally. Of course things won't work the first time—the key is to stick with it until it works.

However with experience and "seniority" people develop expectations of themselves: about speed, about best practices, about performance, or any number of other methodologies. Basically, you expect a lot of yourself. These expectations often have the effect of slowing down exploration (i.e. I'm going to do it right the first time) and can lead to negative-valence feelings when things don't work out (e.g. you have an unproductive day, run into lots of bugs, or can't figure something out).

A beginner will be fine with such setbacks because they don't have a big ego, but a "senior" developer will be—literally—hurt by such setbacks, and they shouldn't be. It's all good. Failure is normal. Just get up and try again.

Deleted Comment

StillBored · 5 years ago
"New Tech Is Just Recycled Old Tech"

Without quoting the entire paragraph, I'm not sure "C" has ever been the mainstream choice. At nearly every point in the history of C, there has been a "better" or more popular language. Be that jcl, cobol, fortran, visual basic, java, python, php, C#, etc.

C/C++ are systems programming languages, they are the underlying technology supporting all the business and application development. Even when I did less pure system programming, our applications were frequently split into a lower level/server/etc portion and the higher level user facing code. The former was written in a high perf compiled language, the user facing code was generally written in a higher level language.

And that is still true today, it takes very little google foo, to find project after project which decided they needed higher perf/more efficiency and rewrote the core data handling in C. Even in cases where C/C++ weren't the obvious choices (https://github.com/sass/libsass).

And yes, sometimes one ends up picking a chain of subpar languages and rotating them du-jour. But in that direction you get trapped on both some language everyone hates (facebook/php anyone? although to be fair facebook has made php one of the better choices these days) So much so, that if I were to start another mobile application, I would probably use one of the C++ based toolkits. Because those are the only choices that provide good cross platform UI's that are performant. For everything else I would probably just write a web application with decent local storage caching/etc.

So, its probably an unpopular opinion, but as a senior developer, I think the ability to make unpopular choices, particularly when it comes to language and toolkits is one of my strongest attributes. Rarely will you find me picking something like react-native (or electron) for mobile apps because i've yet to see a case were the benefits actually outweighed the negatives. Similarly for native toolkits tied to a particular platform because i've yet to work anywhere that could legitimately afford to double (or worse) their engineering efforts by writing the same application multiple times.

mhitza · 5 years ago
I actually disagree on the third point.

I'm back at implementing web design the same way I used to do them back in 2007. Sure, instead of tables I'm using div + flex and instead of inline styles I'm using utility first CSS. For all intents and purposes it's exactly the same process, with just another coat of paint.

The major difference is that back then in order to write terse CSS I memorized all the short forms, whereas nowadays I'd have to look those up.

corebuffer · 5 years ago
I think third point is more on not despising new tech, but I agree we should also remember not to do the same to old tech.

I learned a lot with Forth, which is older than me. While that is true, I can't also deny that Rust improves a lot on embedded development and is newer.

While I can argue that using Forth or C is possible and simpler/easier, the lesson seems to be that ignoring Rust isnt a good idea.

IMO its like you can keep using raw CSS instead of the latest hype, but the key is not to ignore and always evaluate new things with the same care as you did to old ones. :) (at least that is how I understood it)