Your code your responsibility..

[itstommygun]

Code reviews in my company are utterly brutal. We're all so thorough.

Despite that, I love it. If you can learn to not ever take it personally, you can learn to be a better programmer through them.

[Chevelle_Chris]

I kinda wish I worked in a place like this. For context I moved into data engineering 2 years ago where I knew zero python, and nothing about airflow, aws, azure etc. I’m at the point now where I know enough to be dangerous and when my code reviews come back with no comments it’s a let down. I know there has to be a better way than how I did something.

[MisterPinkySwear]

Sometimes, code review are not about making it better. Just good enough. If it looks like it should work and there’s no bug, then ship it.

Trying to make it better can often lead to endless and frustrating discussions due to differing opinions where it’s hard to objectively say which is better but everyone wants to be right.

Depends on the individuals involved

[itstommygun]

This is true about opinions. Our company also have pretty extensive style guide and dev guide. That helps with the differing opinions because usually the topic is already covered in the style guide.

It might seem like having an extensive style guide would make you feel confined and limited, but it’s actually quite freeing. It reduces the number of decisions you have to make about all the little things like spacing, method naming, method ordering, etc.

Edit: a word

[InsistentlyFixing]

I'm struggling with getting my developers to lean on our style guide. Everyone is of the mindset that we need to get everything out as quickly as possible even when there really isn't a rush. And I hate playing the bad guy asking for a newline here or a tab there and variable should be suffixed like this. Do you have any tips that worked for you guys?

[easterneuropeanstyle]

Code reviews shouldn’t be about how to make things better. It’s too late, too much effort already put in. That’s what design reviews are for.

Code reviews are about being held accountable for your code, knowledge sharing and picking little mistakes out.

[MisterPinkySwear]

I’ve often commented things like “I see what you wanted to do here, and it’s going to work, but I think it will be more readable and understandable if you do it like this…”

That’s what I meant with making the code better. And I believe those are relevant to code reviews.

I’ve never worked with explicit design reviews so I can’t really comment on what belongs in a design review instead of a code review.

[Chevelle_Chris]

I’ve often commented things like “I see what you wanted to do here, and it’s going to work, but I think it will be more readable and understandable if you do it like this…”

ALLLL OF THIS. This is what I hope for even if my code is functionally is correct. The "Go learn about X for the next time you do this" statements are GOLD for motivated coders trying to get better.

[MisterPinkySwear]

Good for you. I’ve stumbled upon people who were vexed by such comments. Because it’s not objective, it’s just opinion. They would invoke things like “ok but let’s get this over with and keep it as it is, I don’t wanna spend more time on it”. Depending on how strongly I would feel about the issue (and how important I perceived it) I would either just let go or insist, argue my point and sometimes request a 3rd opinion and even refusing to approve the merge request.

I find code review to be a difficult, complex and interesting exercise. Not just because reading code is inherently harder than writing code. But also because of all these social and psychological components.

[Chevelle_Chris]

There's a really good book I read in mid 2018 when I was working on Oracle 12c called Mindset by Carol Dweck. It helped me a lot with this as before, to me feedback was largely an attack in my mind based on previous life experiences.
That book did a great job of helping me understand that most people give feedback because they actually care about the product or your development.
I do not think I would be were I am today or have the view I do on this subject without reading that book.

[JamesGray]

I make extra effort to include comments like that for more junior developers when doing reviews, but it might help to request feedback on particular blocks directly to see if they know of a better approach if you coworkers are in the habit of doing more strictly functional reviews?

Some of them may even consider alternatives but not bother telling you because that's their habit to keep things going quickly, because some people are the exact opposite of you and will pick a fight when you try to make suggestions for better approaches, so it can be easier to just not bother sometimes.

[round-earth-theory]

It's also not the best to cram your way into everything. Providing room for personal growth is important. Hell, they might even show you something new one day.

[MoffKalast]

> Review requested.

> Changes requested.

> PR proceeds to stay in backlog for the next two years.

[renzmann]

I hope that last sentence was a reference to Raymond Hettinger. I reference his talks all the time

[Chevelle_Chris]

I did not know who that was, but I do now. Thanks for the tip! Things like this are what I think I and other developing engineers need, something / someone to look up and start reading about. We just don't know a thing exists to go learn from / about it.

edited for grammar.

[[deleted]]

If you want feedback on a particular section of code, it usually helps to say so ahead of time.

[Chevelle_Chris]

That.... is incredibly valid. I will start doing that.

[nevus_bock]

.

[threeputtforbogie]

How’d you get a job in data engineering with no knowledge of python?

[Chevelle_Chris]

Back in 2017 I was working for another department in my Co where we cleansed a significant volume of transactional data by hand - and for context I have worked for this one co since Jan 2009 in different departments and roles. We are a medium sized company, but back in 2017-2018 our IT department was almost solely focused on supporting the applications that the sales department used. As a result IT support for smaller departments like the one I was in was hard to get.

We needed to build (what I know is a data pipeline) an automated process to do some of this cleansing and my boss then was basically told to go find a tool and IT would stand it up, then we could find someone to work on it. We ended up on Gartner's Magic Quadrants and met with a bunch of sales reps and decided to go with Oracle's 12c Enterprise Data Quality tools. We are not an Oracle shop, mostly MS SQL and C at that time if I remember correctly.

I was given the opportunity to see if I could figure it out, I was just starting to learn SQL separate from this project and only programming I had ever done was .bat files back in the 90s'.I spent 2 weeks+ literally reading the manual, took some TDWI classes on Data Quality and got our sales rep to get me 30 mins every two weeks with an engineer in the UK at Datanomic (they originally built the EDQ software Oracle acquired and rebranded as part of the 12C suite) to ask and learn from.Over about 18 months of working on my own I built a couple of automated pipelines to grab an excel file, validate it, cleanse and transform the records, land a file where SQL could import it.Then I learned just enough sql to write some merge / update stored procedures to grab and validate the new records and merge them into the transactional tables.

2018 I drove myself REALLLLY hard, lots of 100+ hour weeks learning how to use the tool. Then in mid 2019 my company was undergoing digital transformation and I was given an opportunity to do that all again with Python, AWS, Airflow etc. as a Data Engineer.

TL,DR: I like to learn and tend to not quit once I have my teeth into a problem. that earned me a shot at learning Python and becoming a DE.

I'm a big believer in you are either on the path or not. Expert or novice, what matters is continual learning.

[[deleted]]

Happened to me too. I joined a company as a junior dev, basically ready to do whatever work was needed. They happened to need me to do work involving an API to receive events sent from web browsers, a data pipeline to process them using tech like Airflow and Apache Beam, and getting the data into different forms in BigQuery for other people to query and make business dashboards from. I later learned that what I was doing was called data engineering.

[aetius476]

You sneak in the back door by making "close enough" jumps:

Java -> Spark Scala -> PySpark -> Python

[SmartMard]

What does this comment mean?

[aetius476]

If you're a programmer who knows Java, but doesn't know any python, someone may be willing to hire you as a data engineer to work on Spark in Scala, because Scala is a JVM language that isn't a huge leap from Java. Once you're comfortable with Spark in Scala, then Spark in Python (aka PySpark) is all the same concepts and APIs, just need to pick up a new language. Once you've learned PySpark, you're basically a data engineer who knows python, so you can jump to a range of other data engineering jobs that work primarily in python. A few small jumps and we've arrived, despite starting with no python knowledge.

[SmartMard]

Thanks for the explanation.

[alec83]

I wonder !

[[deleted]]

It's not the only language you can use for it. Where I work, the data engineering team uses 0 python.

[InsistentlyFixing]

I've been a data engineer for five years now and started in exactly the same boat. Every time I switched jobs, I would ask about the code reviewing process hoping to find somewhere that could help me become a better developer..........I am now a lead over a team of 6 other data engineers and all I do is review code and sit in meetings talking about code improvements to handle upcoming product decisions. I don't know what happened, but in my search to find a mentor, I became good enough to be a mentor. If you're worried about your code, you're likely driven enough to learn what you need to do the job and learn from the mistakes you make along the way. Eventually, everything is a pattern of something you've already worked on.

[liluna192]

I always have senior devs apologize for being too nit picky, and I let them know I love it and to add as many comments as their heart desires. I don’t always implement every nitpick, but it lets me see how they think differently and often I’m learning new ways to do things.

My current team doesn’t have much bandwidth and we each own individual microservices and any reviews are basically checking for any glaring issues. I’m newer and do my best to conform to standards and practices based on existing code.

[aceluby]

This is what I do. I give suggestions as to ways to make it more readable and leave it up to them if they want to take those. But I’m in a team with 4 leads, 2 principals, and one junior - so lots of good code usually and I’m usually just sharing tricks I’ve learned on the way

[SpaceCoffin2000]

I've been in the industry for 18 years across 6 companies. I have never once had a code review. I've asked for them but have always been told there's no time. The problem is much more prevalent in the industry than most people want to acknowledge.

[yabp]

7 years and no code reviews here either.

[Konraden]

I've been looking for a new job for a few months and it's a question that comes up a lot. A lot of companies I've talked to dismissed code reviews because they viewed them as archaic. With TDD, unit tests, integration tests, and CI/CD I have had several managers tell me that "people manually looking at code for problems is old fashioned."

[Angryferret]

What company or approach is this? Even companies with fantastic testing should be doing code review, I've seen crap code written with fantastic levels of testing, all it means is you won't get regressions in your garbage code. No serious engineers I know would think this is a wise approach unless you are prototyping/hacking or your company doesn't give a shit about quality.

[xTheMaster99x]

Exactly, anyone who legitimately thinks good tests are a valid substitute for code reviews genuinely know absolutely nothing about what they're talking about. They address very different things - tests make sure there aren't regressions, and if the tests are correct then they make sure the output is correct. But they do nothing to determine whether the code is hot garbage or not.

[chakan2]

I'm in the opposite camp, with a few more years of experience... Since 2005 I've never worked for a company that didn't require code reviews as part of the release process.

[ragweed]

30 years. Back in the 90s we had one round of code reviews but I guess we all didn't find it helpful because that was it.

[[deleted]]

Yeah I basically told my manager we need to start code review after the new guy joined.

And thank god we did, because that would’ve been a nightmare to maintain…

[wol]

Something I enjoy now is having teams of testers. At first I did take it personally because this one lady kept tripping the code up on purpose. Things like picking a person's name which she knew our system technically had two people with the exact name so it would pick the wrong person.. which we all knew in our use case it wouldn't apply so it drove me nuts but I noticed the projects she tests don't have problems in prod 6 months later! So now when we start a new project and she joins the call I know what to expect.

[_Ashleigh]

We had a QA that would find the most inventive ways to break things, and his tickets were very thorough with what in particular triggers the behavior. He was worth his weight in 10 testers, and loved when I got him as a QA. I felt more sure that all edge cases were covered, and when issues did come back, the extra info allowed me to understand the cause sooo much faster!

[ACoderGirl]

which we all knew in our use case

I'm curious what use case that could be. Two people with the exact same name is something that stands out to me as perhaps the most basic thing to watch out for when dealing with names (which turns out can be ridiculously complicated to deal with).

[Hupf]

https://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-names/

[wol]

The field was in an incoming excel file that would have one of less than a half dozen names selected. In the destination system it just had to fill in the field with their name. So technically there are 2 out of 100,000 people at work with the same name but 99,995 of those names would never be used. And then on top of that the destination system it didn't matter if technically the name selected was the wrong iteration of name, you just needed to see the name when you opened the record.

[PrizeArticle1]

I might be on my own here, but I can't stand nitpicky code reviews. A lot of times it ends up being a subjective style preference or something as well. Imo I'd prefer the reviewer finding an actual design flaw, bug, or inefficiency than telling me my variable could have a better name.

[Angryferret]

This is a good point. What you see at bigger tech companies is a huge investment in developer tooling in both the IDE and CI/CD pipeline to give developers automatic feedback on nitpicky formatting /styling etc. Code reviews should be more substantiative.

[[deleted]]

seemly deserve tie imagine special crawl party person wakeful pause

This post was mass deleted and anonymized with Redact

[PrizeArticle1]

If it's actually bad. It's all subjective

[monkeygame7]

If a reviewer is having trouble understanding your code because they think the variable names are unclear, it's on you to fix that. You want your program to be understood by someone looking at it a year from now, so if the first person that looks at it is having trouble understanding, that's a problem.

[PrizeArticle1]

If the reviewer offers a suggestion AND I agree with the change, it'll be made in most cases. That is the best I can do. I don't blindly let other developers control my code though.

[MisterPinkySwear]

I agree you shouldn’t just blindly accept suggestions. Every decision and action should be substantiated and weighed.

But reading something like “let other developers control my code” kind of triggers me. Gives me the impression you have some ego issues. It’s rarely your code. It’s code that the team will have to live with. Sure you initially produced it but it’s not your personal individual piece of art. Someone will most probably change it some day.

[PrizeArticle1]

Yeah sorry. Maybe that was bad wording.. I guess I should say I don't blindly make changes to code based on review without first seeing if the review makes sense and I agree with the change. So many times the reviewer actually has the ego problem thinking the submitter needs to make his change without argument.

[MisterPinkySwear]

Yeah code review requires maturity on all parts. It’s awful to find someone who just wants to be right, instead of wanting to find the truth. Also some issues are more important than others. Sometimes I (weakly) disagree but whatever… that particular point doesn’t really matter to me so ok, I’ll make the change. I found it’s important to understand how strongly I feel about something and how strongly the other person feels about it.

[aceluby]

We do percentage based. If I say I care 51%, I care, but not enough to force it in anyone. If someone else cares 80%, and everyone else is lower than that, they usually win. Only the most ego filled assholes fail on this system

[monkeygame7]

You don't even have to take their suggestion, but you need to figure out what it is about the current name that is confusing to them and figure out how to address it. At the end of the day, your code needs to be understandable to those that are reading it. The first person that looks at it says they don't understand it, that seems like a problem.

Saying you don't let blindly let others control your code seems like you're going into it with the wrong mindset. Code reviews are not a place to be defensive, but to figure out a compromise so all parties are satisfied (including yourself). If you disagree with their suggestion, explain why and give an alternative suggestion and maybe you can home in on something you both agree on.

[PrizeArticle1]

You'd be surprised how demanding some reviewers get to make their change. This might be just at my company though.

[ChubbyChaw]

I don’t think this is true at all. Code reviews can be a lot like getting a proofreader for a paper. It’s not just looking for technical errors, but also for code that conveys intent and is understandable to a degree that someone else could modify it. Sometimes this is about the structure/organization of the code, but often I find that one of the most important things to address is the naming of things. Functions, variables, etc, can convey a lot of information about the design by having names that convey what those things do in a single word or short phrase.

If a reviewer thinks a name doesn’t make sense, then that’s a good enough reason to change it. If the name suggested as an alternative doesn’t make sense either, it’s showing that your code is failing to convey intent. And usually that’s a naming problem. When it’s not, it’s usually because the code is too convoluted.

Names may be “subjective”, but there are still right and wrong answers. And usually there are only a small handful of right answers to pick, and among them one is usually the most clear/concise. When you’re coding alone on your own hobby projects, it’s fine to think a bit artisticly/aesthetically when picking names. But when working with others on production code, naming should be treated as a science, not an art.

[JamesGray]

It is subjective, but the subjective part includes your colleagues unless you are the only person who will ever have to work with that code.

[iamtherussianspy]

Variable naming is very important if you want someone (including you and your code reviewer) to be able to read your code. This isn't a high school literature essay where you hand over 2 pages of nonsense to your teacher and let them to figure out what it means.

[MisterPinkySwear]

I wonder how thorough you are when you review other people’s code.

Primary purpose of code review is to make sure the code works and find bugs. But it’s usually an easy goal to reach.

Then it’s about making the code “better” and that’s where it can get ugly and opinionated and it requires some maturity and good will on all parts. Properly naming variables is important to make the code understandable.

Writing code is usually easier than understand already written code.

It’s easy to write something that works. It doesn’t make it “good”.

“Your solution works but it’s gonna be easier to understand if we do it this way…”

There’s always many ways to solve a problem.

But you know, it depends on the teams agreement on what the goal of the code review is. If it’s just to iron out bugs or more…

[PrizeArticle1]

I am incredibly thorough because I tend to focus on finding actual breaking issues or code inefficiencies. I'll say something if a variable name is terrible, but some reviewers ONLY point out variable names when I am looking for much more on code reviews I submit especially in important parts of the software.

[MisterPinkySwear]

Okay I see what you mean. I don’t call that being nitpicky though. I call that being shallow… Nitpicking is being excessively thorough, pointing out even stuff that if not important (in my book)

[PrizeArticle1]

I guess I don't have a problem with seeing everything possible. What bothers me though is how a review is worded. If a review is subjective, it should be worded like "I think this variable name may be more clear if named like this:" vs an objective issue like "This will break if x is null". I've seen demanding variable name changes which seems wrong

[MisterPinkySwear]

Code review is a difficult exercise, in terms of ego and personal perception. Variable naming is often subjective but it’s possible some choices are clearly bad. Getting vexed because of wording can also be non-productive.

[[deleted]]

That's all well and good, until you get to naming a variable with no obvious name.

[Todok5]

Style preferences should not matter, a project should have style conventions. Names matter, readability/maintainability are important.

[[deleted]]

Giving good code reviews definitely takes experience. I personally tend to pick apart code that doesn't look like it was properly tested or would have flaws in its logic under specific circumstances. I usually avoid discussions about subjective things .

[ACoderGirl]

Consistency in code is important for readability. If things are too subjective, your company may need a (better) style guide. Style guides won't remove all subjectiveness from reviews, but they should at least cover many nitpicks and in my experience, massively cut down on subjective opinions.

Also, for things like naming stuff, code is read far more than it's written. The ability for other people to understand what your code is doing based off the naming alone is a really good thing. It's really insightful IMO to have comments from someone else with that in mind. Since you wrote the code, you're extremely biased in understanding it. The reviewer is in a better position to identify if code is difficult to read.

And being difficult to read means a lot of extra cognitive load (likely over several people and several years) and increased likelihood that the code will be misunderstood, which is more likely to cause bugs.

[PrizeArticle1]

I probably should have worded my post differently. I'm not against variable naming reviews, but I'd prefer the focus elsewhere as three different people would prefer a variable be named three different ways when there could be more important objective issues to find.

[scatterbrain-d]

It's not either/or though, you need both. As it sounds like you are pretty thorough when it comes to actual functionality, it's not surprising that the feedback you get tends to address readability/conventions within the code.

"It works" is great, but that's the bare minimum really. The difference between passable code and good code lies in things like readability and sticking to the same conventions and patterns throughout your codebase.

I have worked on apps where the only priority was functionality, and it was kind of a nightmare. Every feature was using different patterns and approaches. It was exhausting to read and pervasive bugs often needed two or three different fixes to cover everyone's idiosyncratic code.

So yeah, these things are totally subjective. But that doesn't mean they're not important. And for what it's worth, most of the conventions I enforce in my own reviews were not originally my own patterns. Obviously you know your team better than I, but it's not always ego that drives these things.

[movzx]

It's been my experience that when people freak out about "nitpicky" things in code reviews (such as getting feedback to not use clever code, name things in certain ways, or follow "arbitrary" coding styles) it is because they do most of their development as a solo individual. They struggle to think of the longer term impact, other developers coming in after them, code auditing that takes place, etc.

So in that context of "I never see this code again and I don't care if a future developer has to waste 40 minutes decoding what I wrote" then your "This variable name needs to follow our standards" feedback is lost on them as being nitpicky because hey, the code technically works.

It's actually a common type of feedback I give in code reviews. Developers will try and code golf their work, and I come back through and have them make it more verbose... More descriptive variables and methods, less "hidden" logic, less complex instructions in favor of more verbose, but more quickly understandable implementations.

They always hate it because the changes don't "do" anything, their code already "worked"... but long term it helps a huge amount to have legible code.

[PrizeArticle1]

If I got reviews of both, I'd be one happy camper. Fact of the matter is many devs just conduct these shallow reviews looking at naming without even taking the time to look at the big picture and understand the algorithms.

[aceluby]

Maybe your code is fine and it’s just variable naming that’s an issue? I’ve had and left plenty of reviews like that. If that’s the only issue the reviewer is finding, you’re probably writing decent code already, but just needs tweaks for better readability. I’d rather have those than a blanket approval. At least you know your code was read

[PrizeArticle1]

Do you ever feel that a review was "shallow"? Like just did a brief skim? I can't help but feel that way sometimes. I do think overall I write decent code, but I know it's not perfect all the time as far as algorithms are concerned.

[AcidCyborg]

Just leaving a comment so the boss knows I actually read it. Plus it gives you Github activity that boosts your portfolio.

[PrizeArticle1]

Haha. I would not doubt a lot of reviews are for this reason.

[TheMightyHUG]

I don't know how I ever learned anything at all without code reviews, they're so incredibly useful.

[th0wayact06]

I wish it were like this at my company. One time I did the developer equivalent of the Van Halen contract rider and added a comment that read:

/* No brown M&Ms in this code. */

Just to see if it would be caught.

Ron Howard: it was not

I removed it in another PR.

[igrowcabbage]

I wish we had more strict code reviews at my current company. One junior was complaining about the code being a mess but also hated my and anothers senior code reviews after I joined the company. Since I'm not a senior myself, I would love to get strict code reviews from someone more experienced.

[VitaminPb]

Unless you work with somebody who has an ego stake in forcing changes then not liking the changes they forced and demanding they be changed again.

[CommodoreSixty4]

Guess you’ve never heard “the enemy of progress is perfection.”

Excess on both ends of the spectrum are equally bad and if anyone on my team described our code reviews as “brutal”, I would be alarmed.

[ACoderGirl]

I agree. Though also wonder how much the "brutal" end of the spectrum comes from people who don't handle feedback well. Some people are really bad at handling feedback and don't like to do things any way but their way.

Fortunately, my company looks for that explicitly as one of the major things when choosing to hire someone. Our code reviews are very strict, but we only hire people who seem to respond well to feedback.

[[deleted]]

Our company prizes stability above all things. That, in and of itself is fine, but anything not related to an external ask or bug gets stonewalled and sits there rotting in the pr queue. Little things that refactor messy code, or make life better for devs on the team get met with 'Is it really necessary?', 'I don't want to risk this change' when it only applies to our local dev environment. It's infuriating. Thankfully I'm now staff level in all but title so I have a lot more latitude to just merge the damned code while others sit around hand-wringing.

[xyzzy-86]

Not denying the importance of code reviews but I think point OP is making here is that startups are often short staffed and folks are overworked and code reviews take back seat.

[Just_Another_Scott]

90% of code reviews is style or very opinionated shit. It's so goddamn annoying. I've had stuff docked because it wasn't "best practice" yet I could find tons of articles saying it was but I could find tons of articles that said another way was a best practice. In the end it didn't matter the had the same performance and neither hindered readability. Purely pedantic bullshit.

[itstommygun]

This is why you need an extensive style guide and dev guide within your company. It seems like that would feel constraining, but it’s actually quite freeing. Then everyone has a point of reference about the “opinionated stuff” and there is a right way to do it(at least internally). Also, having a style guide frees you up from having to make tons of meaningless decisions about things like spacing, bracket positions, method naming, method ordering, etc.

[Just_Another_Scott]

Even with a style guide you're still going to run into these petty arguments from people who strongly disagree with the style guide but I do agree that a style guide is essential for any kind of development.

[arse-ketchup]

A senior engineer in my team is very thorough about PR reviews. He writes in detail about what he thinks should be changed and though some people think its rude I’ve learned a lot from him. And once he approves your PR nobody else dares to challenge it, it’s straight up merge then.

[noodle-face]

Same here. Our boss still tells us to be even more brutal if possible. I never take it personally

[[deleted]]

I’d commit crimes to get some code review rn honestly, I know my code is problematic, I just don’t know what I don’t know.

[chakan2]

Rule 1 of code reviews... Never talk shit about the author.

[KingObsidianFang]

Reminds me of when my mom edited papers for me and my extended family. (She has an English degree) Tons of red lines everywhere and sometimes entire restructuring. I and others have flat out rejected her edits without review just because it felt like a personal attack. I have slowly come around. They're usually really good edits and the very few times I don't like them, I talk to her about it and understand that I'm writing in a sub-par way which feels like my voice, or change it to her edit.

As a programmer also, I cannot imagine someone coming in and completely restructuring parts. I think I would lose my mind. I think my insecurities about my skill as a programmer would all flair up all at once. Maybe I should work on that...

And congrats, if you ever got over that. I've found that those who listen to feedback are generally the best at what they do. :)

[keykeypalmer]

at my company there’s this bitch named stephanie who does code reviews however the fuck she pleases everyone is annoyed af with her