STOP Nit Picking In Code Reviews

hey stop nitpicking in code reviews you know how many times I would receive these nitpicks that I absolutely hated my worst nitpick that I hated the most the absolute most was that we had a convention or at least someone had a convention and I did not agree with the convention I refused to do stupid conventions and one of them was to underscore your variables that are private in typescript and I said yo dog it's a typescript library and typescript disallows you to access private variables and their response was well actually you can by doing at at you know at TS ignore and I said yeah but if you're doing test ignore you're fundamentally doing something wrong stop underscoring something that doesn't exist anymore that notion that idea comes from a time when we just had JavaScript now that we have private and public modifiers you don't need it anymore it's stupid to do that don't do it okay if somebody goes off the rails and accesses something that's been marked as private and ignores that your linter typescript guess what that's them being stupid it's not on you okay you can give somebody a gun and they can shoot themselves in the foot that's their problem that's their problem okay it's their problem they pointed at their foot and then pulled the trigger don't do that don't shoot yourself in the foot and so those kind of nitpicks drive me up a tree right it's like no F your conventions F that because it makes no sense we're naming the variable without that okay one of the best changes I've made at work recently is to stop nitpicking uh in code reviews I Dan Liu I love you I love you Dad that's oh I love you oh nitpicking isn't about code that is wrong but sub-optimal it's pointing out a variable name that could be used more appropriate like a more appropriate word a condition that could be formatted more cleanly or some minor simplifying of logic nits don't result in significantly better code nor did they educate the developer they're just changes that are technically improvements if not highly meaning uh if not highly meaningful ones uh I'm not really sure about that last one um I would say that see that's the problem it's like even okay get this here's a good example uh we're in some JavaScript and someone does new promise uh res reject blah blah blah blah I've been nitpicked on this it's just like yo dog yo dog how about you don't how about you don't okay in the context where they're at it doesn't matter or if somebody goes like this right you have an array of numbers right and you do a reduce and you do an ack X act plus X zero right if someone's like yo you should rename X because we don't use single variable names you're just like stop stop yo stop okay ain't nobody caring about that stop you know what I mean it's just like that kind of stuff is just so effing annoying now if your variable name is loads from data source and it actually goes and reads out div elements data deserves a rename deserves a rename because it's fundamentally wrong totally agreed but if you believe there is a better variable name than the one that's chosen it's likely because for your brain you understand it better now some people won't understand it better that's the thing is like people totally forget the fact that perspective comes into every variable name and when somebody does like you know does it one way and somebody else does it another way like a really good example is is in uh you see this more and more is like if somebody has some sort of class with something in it right and you do something like uh time and it returns you know uh this dot current time I don't know look at that nice underscore I did that just to piss somebody off versus get time right these are two these are the same thing and people argue over this now because some people like the former approach better where some people like the latter approach better and this is like totally uh your experience gets you into this place you know what I mean your experience gets you into this place and if you are super used to something then You're Gonna Want A Time versus a get time and that's just that and for me I prefer time I don't see why I like get time Some people prefer get time with the capital G because they're c-sharp losers okay I want a code base to be perfect and in my mind these nits were an important part of ensuring that happened I used to conduct extremely thorough code reviews filled the comments about architecture and bugs but also so so many knits the knits often overwhelmed the other comments for every major concerned I'd raised there could be a dozen nits this is another great Point great great great great point right there this is such a good point when you overwhelm people with a bunch of nits how hard is it to make the proper things fixed you know what I mean if you see something that's fundamentally wrong address it right that is way way better than being like you know what your underscore in that private they were missing underscore in that private variable you're like you just missed the whole thing you know what I mean this is practice of mine uh did not make me the most popular co-worker far from it I didn't realize it at the time but my perfectionism was toxic at best it annoyed people at worst it upset them and those feelings lasted far past the code review itself exactly there's people on Netflix I don't want to work with because they're like this right like I'll avoid working with them which is bad bad on my behalf but there's nothing you can do to get away from it now this is what they're known for is being Ultra nitpicky it's just like you gross I don't want to work with you because here's the deal anytime I go into code I think it looks like no matter who wrote it when they wrote it including if that person was me if it is more than a week old it is pure ass code every single time every single time every time a couple years ago a co-worker proposed that we uh see what happens if we stopped nitpicking altogether initially I pushed back I knitted The Proposal why would I want to allow bad code to get into code bases but in the end we agreed to a one month experiment what a great thing right here I love this also another great sign of a healthy team are people who are willing to go against what they think and just try something out this is awesome to my to my surprise at the end of the month things were cleaner uh better than before were clearly better than before so much so that we barely even discussed whether to continue the policy it was just sort of became a permanent because it was obviously an improvement uh there were two big benefits we experienced this is such a well done article right now Dan Liu you did incredible this is incredible this is this is awesome first we saw a vast improved signal to noise ratio absolutely absolutely um it's just like if you have a CI that's kind of flaky you know a flaky CI and you have that one test that fails kind of regularly who here's merged on a broken test because of that one flaky test that's kind of flaky who here's done it who here's done it this guy's done it right here this guy and guess what one of those times it wasn't flaky it was broken you know what I mean but that's the problem about signal to noise if you have something that's noisy people will Overlook things you know what I mean imagine a code review that gets results in five nits and one critical issue to address in the hullabaloo of fixing those Knits the critical comic can seem less important or even get overlooked not only that but if they ask you to refactor a couple things pull this out as a function or go do that the critical bug sometimes disappears from the code review and will be missed because now you have a restructured version that you can't quite see what is Beast doing one one one one one one one yeah me and bsco were a lot alike not a Hubble a hullabaloo yeah a hollow balloon uh without nits okay what is going on with my voice I can feel something in there sometimes the critical bug is also fixed by refactoring absolutely and sometimes you introduce a new critical bug by refactoring we call that refactoring um without nits that one critical comment is the only issue that gets brought up all the attention is paid to it and right rightfully so what is important gets a signal boost what's important what's unimportant gets ignored yes second it improved everyone's relationship with code reviews and those who conducted them I've seen plenty of people talk online about how you shouldn't take code reviews personally that's uh that it's the code being critiqued not the person blah blah blah blah blah it's a bunch of I've been going through code reviews for a decade and it still stinks when someone points out my mistakes or pushes back on my code designs imagine used I I actually don't really feel this too much I just don't care like that to provide a good reason for the most part I think that most people when they push back it's just a bunch of nits and then I don't care imagine you start a pull request and the first reviewer points out a dozen nits your natural response will be uh will not be zen-like acceptance it will be to get upset uh all you want to do is merge this code but some jerk is making you jump through a bunch of dumb Hoops to do so first now or suppose now that you fix a nitpicks and send it back to be reviewed only to get blocked again because the reviewer noticed some more nitpicks yep yep developers like to imagine they are are composed of nothing but cold hard logic but actually we are humans with unavoidable feelings and emotion uh what I what was what was I really doing then or when I was thoroughly nitpicking code the main effect wasn't improving the code base it was making people upset with their uh code and angry with me I'm so motivated by this uh I think uh oh my goodness I have a tweet coming I have a tweet coming I can't do it yet it's not there yet it's not I gotta get sufficiently more upset hold on I gotta get sufficiently more upset at what I'm about to say and then we'll come back to it we'll come back to it I can feel it coming though Something's Gonna Come Out of me I can feel it um uh they were less receptive to my feedback as a result and it soured my relationship with my code workers a couple years after stopping knits people are far more receptive to feedback and code reviews and I think much less of it and and think I'm much less of a Jerk It's a night and day difference from before ultimately our code base has not suffered due to lack of nitpicking if anything it is better than before because we as a team work together better and focus on the important parts of the code design in our reviews let's go but what if you still care about what uh what you nitpicked before my suggestion is to automate what you think you'd otherwise nitpick add lint checks and code style enforcers to your CI absolutely this is like the simplest thing is just agree on a set of prettier rules you know you're gonna have to push back on some things you and at the end of the day somebody you have to designate a time box for argument and a way to resolve all conflicts and just do it you will argue over any rule no more than five minutes and if there is a pure tie you flip a coin right and that's just that and you move on and and just who cares right I'm a four space indenter some people are two space indenters I think two space indenting is dramatically worse for a code base because it allows you to indent more without feeling the pain of indenting I also only choose 80 line characters right here okay I when I well this is templating templating you have no choice on and HTML you have no choice on but in JavaScript I enforce 80 lines why because it makes me think about refactoring much much more it's a personal opinion now some people don't like that some people like tabs I'm a I'm a Force Base indenter that's just the way it is uh it will always be that way 40 40 and 80. hands down the 480 is the best uh with tools you can check problems in advance plus when automation tells you you're incorrect it's impersonal and a result of somehow less frustrating yeah it's just because it is what it is but seriously if uh you're like let's see if you are like how I used to be someone who wanted every line of code to be perfect just try letting it go a bit check your a low Gaff before you comment what the hell's a low Gaff it's another damn can you just tell me what the hell it is what the hell is a low gaff level of give a ah yes I think everybody needs to check that level and adjust it a little bit you know what I mean because here's the deal is that the only things that are going to screw you are bad Logic the only things that are going to screw you are really obnoxiously long functions that need to be refactored that's it and you know and lastly the thing that's going to screw you is uh too much unit testing or not enough unit testing so I built like recently here's a good example I recently built a bit of a Conway a Conway Game of Life thing and to make sure my Conway Game of Life stuff works in this little JavaScript thing I was building I just put a simple test I wanted to prove to me that I could do Conway correctly and this didn't cover all the cases uh but it covered enough of the cases for me to be happy right and so for me that's a sufficient I like I don't need a ton of tests to tell me when things are wrong I could have added one more to have the four or more population will kill out uh you know a pixel I didn't good enough don't care you know what I mean you know what I mean yeah I feel like people sometimes way over test things there's like 1800 tests to test every possible case of every little thing I'm like you know that's not the hard part you know the hard part isn't there right you've done something or people test like the dumbest functions have you ever seen that where someone will test like like just like a literal for loop it's like uh function Foo and it's just like uh four let I equals zero is ISB less than ten oh my goodness 10 I plus plus uh return if uh you know uh if I equals five oh return drill return false I mean this is terrible code but you get the idea uh it's just like stop do we need a 10 000 test to run I I don't care about code coverage I care about code that's hard okay if I can't get this right first try I tested pretty much my general rule of thumb and so I don't know I have a bunch of things where I think that people go off the rails and what they require and then it just ends up being such a huge pain in the ass at the end of the day it's just one gigantic ass pain and it really never made your code base better we have we have we have tests at Netflix like 10 000 of them and 8 000 of them run every single time and they provide nothing they've never provided anything they've never provide anything they'll just keep on being tested because that's just what it is and I feel bad about that I don't want that there you go that's that that's my personal feeling get out of here don't nitpick so much okay stop nitpicking the name is don't nitpick my code or I'll ignore you I'll block you on Facebook okay

Channel: ThePrimeTime

Views: 181,058

Keywords: programming, computer, software, software engineer, software engineering, program, development, developing, developer, developers, web design, web developer, web development, programmer humor, humor, memes, software memes, engineer, engineering, Regex, regexs, regexes, netflix, vscode, vscode engineer, vscode plugins, Lenovo, customer service

Id: 08NlhU4gzdY

Length: 14min 5sec (845 seconds)

Published: Wed Aug 30 2023