Soft Skills Engineering - Episode 20: Stories from people who got fired and doing effective code reviews

Episode Date: August 1, 2016

In episode 20, Jamison and Dave share some stories from people who have been fired. We also answer this question: How do I make code reviews more effective? It feels like reviewers fit into 2 categories: either they are too quick and superficial, or they get bogged down in nit picks.

It takes more than writing great code to be a great engineer. This is Soft Skills Engineering, episode number 20. I'm your host, Dave Smith. I'm your host, Jameson Dance. Together we are your hosts. I am half of your host. Hosts, plural, is kind of a hard word to say. We don't English good.

Yeah. All right. Hey, back in episode 17, I think, we talked about being fired and people bouncing back and stuff. And one of the things we raised during that issue was that a lot of people don't talk about this. And so we actually had several people write in to share interesting stories about them being fired and what they did to get fired and then what happened afterward. So we're going to share a couple of those with you now. Jamison, I think you might have the first one. I do.

So this is from an anonymous listener. uh and here it goes i listened to your fired podcast and wanted to yell at the car stereo my fired story i got fired with cause for insubordination because i was an entitled brat my friend who got hired shortly before i did got a promotion i felt like i deserved uh but he deserved it and i so did not and i tried to lead a rebellion against him oh and oh and i worked at the help desk and i did like eight hours of actual work per week and then it says lol eventually it all caught up with me i brought my snowboard to my place of employment for the company ski trip and i got called to hr as everyone was leaving and they were like officially it's not

a good fit unofficially it's insubordination and also you are not good at customer service i was crying and like you don't have a mortgage to pay totes unprofesh about the whole thing i'm pretty sure the words totes unprofesh are tongue in cheek totes unprofesh is the most profesh way to say that totes profesh uh i called my wife bawling on the drive home i'm so sorry i messed up gave it a day and called my friend boss the next day to tell him he was right to fire me and hope we could still be friends cashed my 401k in a panic bad move oh and went to interview at apple retail um told them everything about getting fired like i really owned it and relished it for the learning experience it was out of 2500 candidates they hired 15 and i was one

i showed up and the gm was like hey it's fired guy fired guy hey fired guy and it ends with i learned the customer service skills at apple that i wish i'd had previously so it sounds like this was really a i i like how this person totally owns it and says it's completely their fault but also used it as an experience to improve a lot yeah and and when he went to interview uh later he completely told the truth and it i know this person and they are a great storyteller but also this is a good story so it kind of helped them stand out it sounds like so um perhaps being fired was one of the better things that could have happened here's what i took away from what if they won the rebellion and they still worked at the help desk to this day and the other guy

got fired yeah then what would their it's like sliding doors or whatever that movie is what would their life be like so here's my takeaway from that story don't make important financial decisions right after getting fired yeah don't don't cash your 401k if you can help it okay all right i have another story from a listener also anonymous uh he writes i was going to be placed on an extremely tough probation and my supervisor told me that i probably wouldn't make it through it part of the thought about trying to salvage that situation however i realized is that it would be easier and less stressful for me to start brand new afresh at a new employer. There was a lot of different things that led to that situation. The organization itself had its

own issues. Like in the two years I was there, I had three different supervisors, and we used four different time tracking applications. Everyone loves a good time tracking application. That was just my little note there. I had quite a few projects that I was placed on that put me in a situation to fail. And either the requirements were not correctly gathered, or I was working on some legacy code that had issues. I'll admit around this time, I'd gotten pretty burned out and I wasn't staying up to date on my tech skills. By that time, my third supervisor had come along and he wanted me to redo things and saw some of those situations as reasons to get rid of me. How did I handle this experience? When searching for new jobs. Oh no, sorry. In the end, I ended

up at a great company that provides money for training and has a career path for their workers. How did I handle this experience when searching for new jobs? Thankfully, I started to see the writing on the wall and had started a search i quit on a tuesday morning and i had two job interviews lined up by the end of the week in my interviews i didn't mention anything about quitting i told them that i was still there and that i didn't want them to know i was looking a few weeks later i was hired on and i didn't have any issues it's been almost three years at my current job and nothing has come from how i joined overall i would admit i had i was a tad dishonest in how i interviewed the way i saw it i could either be an honest guy that didn't get a job or i could be

a dishonest guy and get a job oh man it's a unfortunately i definitely lost some confidence and my skills from this experience it probably took me a few months to get over the situation thankfully i gained them back by working and studying so there you go i wouldn't call that dishonest by saying that you were still working there even though you had quit oh did they actually yeah i thought i thought he said quit on a tuesday and had job interviews later that week oh okay i guess kind of kind of that is but it's like a little a little bit dishonest yeah i don't know i i like the part about how he quit before he got fired though because it makes me think of scenes in a movie where they like the person notices a bomb and it counts down to like five seconds

and then they just turn around and start running in slow motion and it blows up behind them they like dive out of the tunnel that's what i think of when i when i think of this situation yeah so so good work i like that he uh he was able to bounce back by training again and even though he had a pretty big blow to his confidence and um he said he uh like his tech skills had really gotten a little bit stale um but he was able to retrain and and get upgraded and everything is great now three years into his new job yeah cool so uh you can you can get fired and still be great yep that's what i take away from both of these stories yeah i do too it's good yeah well i mean getting fired isn't good but but life goes on that's true i was just good that you took

something away from the story that's okay that was good i learned from their pain and it was delicious actually what i meant was that you're jumping from a bomb metaphor was good that's what that too that that metaphor applies to so many situations too like it was good uh anyways all So, Jameson, can you read our first question for today? I certainly can. How do I make code reviews more effective? It feels like reviewers fit into two categories. Either they are too quick and superficial, or they get bogged down in nitpicks.

So true. That is an interesting categorization and very common. Yeah, this is a common complaint I hear, that code reviews become a hoop you jump through, and everyone kind of treats them casually, and they don't help that much, but there's still work you have to do, and it kind of feels like developer busy work. Can we start out by talking about if you really need code reviews? Well, you don't, Dave, specifically. Well, yeah, I don't write any code.

Oh. Just kidding, just kidding. Yeah, let's talk about that. Do you need code reviews? For years, for about 10 years, I believed that code reviews were a sign of weakness. What? Yes. Are you serious?

Yes. and i sometimes i'm really glad that i know you now from some of the stuff you say because you're such a nice guy now wow you just gave me a real some real introspection such a backhanded compliment i'm so glad i know you dot dot dot now oh man no literally for years i thought this and i thought look we've hired smart people they don't need to be babysat you know like these people are super bright why do i need someone to come in and like look over their code just they're capable and confident do it themselves and you want to know the someone in one sentence changed my heart on this and it

was someone who had worked at google and he said google hires smart engineers and they do code reviews and i was like the appeal to authority yes classic classic way of knowledge it really made me think like oh so it's not just about smartness and babysitting there might be some other benefits here and i started to realize that we needed this on our team we needed people to share their knowledge and make sure that we didn't have single points of failure like the bus factor you know like well only only bob knows how that function works you know yeah and code reviews are a great way to facilitate that and many many other benefits so we put code reviews in place two years ago and uh it has been a life changer i've absolutely loved it and it hasn't cost nearly

as much time as i thought so was was that your i mean what was your concern with it the time uh mostly the time suck now you've got to remember jameson since you're just a young whippersnapper in this industry we did not have github we did not have pull requests you know that wasn't a thing and let me just tell you code reviews with cvs and subversion it's not happening like there's no they don't help at all sure you know and there was no github there was no git lab um there was none of that stuff so uh it was really hard like it was like a sit down thing where you had to bring your computer over to your co-worker or email them a diff and yeah there were tools out there like review board and garrett and stuff but those are

all kind of i don't know exactly how old they are but they were not on my radar so yeah you didn't want to be like the linux mailing list where you just scream at each other over email over like three line diffs yeah when that was the model for code reviews i think i'm a little justified in not doing them that's true uh okay now are you glad you didn't know me back then james i i wish we had met earlier all the time um that's really interesting i've never thought about the it's just part of the air the industry consensus now is everyone does code reviews right yeah i think so i haven't heard any arguments against it besides you just now when you said you were too smart for them now oh i believe that was

the summary of your point well i shouldn't i shouldn't say we didn't do code reviews we did post facto reviews where we would have like our subversion or get system email the team with every commit and people would put those in a folder and you could read through them and i would read through them all um but it was always post facto it was never a gate you know to getting your code merged sure so anyway huh whatever let's get to the question so how do we make them more effective or let's take it for granted that we actually are going to do them because i think almost every developer i know does them um i think a lot of it is the attitude of the team towards the poll or towards the code the code reviews man i can't even i want to say pull requests yeah we should

probably just use those terms interchangeably yeah shows my my bias towards the github model um i think if the team feels like they're a waste of time and they're busy work then they will treat them as a waste of time and busy work and they they won't be useful i've never had i've done plenty of crappy code reviews and i've never felt like they added much value um more than like a hoop to jump through for the code uh so this is vague advice but if you can show people the value of doing it well then i think that can spread throughout the team um actually at kawali uh my my my previous employer um we kind of were okay at code reviews but not amazing and then just one person on the team decided that they really really really wanted to get better feedback on their code

and the way they did that was put a ton of effort into their pull request write up they put kind of detailed notes about detailed but not a wall of text so that you wouldn't read it about their thought about their design process about the technical aspects and then kind of they even made a video of what the feature was and how to enable it and how to test it and then they asked hey give me feedback on this design and then can you please check this branch out can you please set up your your program so that you can run the feature that i created in this in this pull request and it took a long time actually it took like 20 minutes half an hour to do it all but it was super valuable and i i found some issues that there are is no way i would have

found from just cursorily reading through the code um and and i didn't end up doing all of that every time but that definitely inspired me to kind of up my game with code reviews both what i asked for and then what i what i gave to other people wow that is so cool that is the kind of engineer i love having on my team oh yeah yeah i guess i can say his name because he's awesome and i'm not saying bad things about him this is murphy randall and he's amazing and i love murphy round of applause everybody hooray he's great um so i i mean just just seeing one person really benefit from it can encourage other people now i don't think this uh totally went from zero to 100 awesomeness but it definitely kind of bumped up the level of of effort and care that went into pull request reviews so

in that story you're talking about work that the review requester is putting in to prepare is there anything else like i think there's two sides of this there's the reviewer and the requester what else what else do you think the requester can do is there anything else i mean smaller diffs are always easier you can just recognize that there's that famous tweet that i've seen where they show the dog chasing the laser pointer and then they show the dog with this grid of like 300 laser pointers and it just is frozen doesn't know what to do it's like the one line diff code review versus the 5 000 line code review and there's totally truth in that your eyes can glaze over uh i think you can get around that by

making smaller commits smaller smaller um chunks of of code to be reviewed there's also i mean sometimes you'll do like some functionality and then you'll maybe clean up some styling or naming or just a little bit of refactoring and if you do that it can be really helpful to separate those into different commits and then even maybe name them that way and say like if you want to look at the meat it's in these three commits this other commit that changed like 500 lines it was changing things from tabs to spaces or or i don't know updating to use this new function syntax or something yeah yeah totally and give tips to your reviewers like this is just a white space change there should be nothing else um you know uh link links back to like the jira ticket or pivotal

story that you're working on can help give some context as well so making making sure that stuff is all linked up is really handy yeah basically you don't want to ask someone to just review code without any context yes yes exactly and when you started talking about that tweet just a minute ago with the laser pointers i started laughing prematurely because i thought for sure you were referring to a different tweet oh well there's this tweet by an account called i am developer where it goes 10 lines of code equals 10 issues 500 lines of code equals looks good to me code reviews yep classic sad truth uh oh so another thing you can do as a reviewer if you're um this is a real hands-on thing but like if you've done a pretty big review of sorry a pretty big diff

and you've got some feedback now phase two is you need to implement that feedback so you have two choices here in the git model you can either amend your old commits and push them back up with the dash f or you can add new commits with new commit messages explaining what you did that have their own diffs and i prefer the second thing especially for bigger reviews so that the reviewer can say okay i can just narrow in on the stuff that you changed to address my feedback only instead of having to do a full uh reread of the entire diff from start to finish sure so this is this is stuff you can do if you are the the submitter the changer yeah the requester okay uh anything else that that person can do if you're the one creating the diff you

can walk over to the reviewer's desk and stand over their shoulder until they review it that's a good point how how do you handle i mean what's the expectation that you have for how long code goes between when it's put up for review and when it gets reviewed yeah that's a good question i mean um on our team it's probably a matter of a couple of hours is that we would expect it to be done um you know we have a bunch of automated tests that run against the branch that's being reviewed first so you have you know you have like 30 40 minutes um minimum before that stuff will all pass anyway which that kind of is a long time but we can talk about that later that's not a soft skill yeah ignore it yeah that's right that doesn't exist um i don't know a few hours seems

pretty reasonable what do you guys do yeah i think that's uh that's pretty average i one of the teams um it's like immediate and the expectation is as soon as you put up a chunk of code you can actually go just tap somebody on the shoulder because they value getting stuff reviewed quickly that highly um i don't know if i love that because it makes it easy to get interrupted and thrown off your your flow um so i kind of prefer the couple hours thing but they never have that problem where um changes just linger for days or weeks even because people are busy or maybe kind of scared of it and it always seems like too much work to start right then and then it just hangs around that's when you just type plus one and move on yeah exactly that's that's what the plus one is

for lgtm but is there a way to say it looks good to me but i'm in no way responsible if this ends up causing any problems from me not looking at it let's see that would be w m h o t washing my hands of this yeah okay i like that that's like the plus 0.5 plus 0.5 can't be held responsible plus some plus okay so what do you do if you're the reviewer how do you how do you give good feedback okay so this is really hard to do first of all i don't know why but for my brain doing code reviews effectively is hard and the people who do it really well tend to spend a lot of time on it yeah and one of the things i do to try to get in the right mindset if i really want to do a good job on a code review, is I look first at the description and the thing they're trying to

solve. And I watch all of Murphy's videos. And then I don't look at the code. Then I take a step back and I think, how would I code this up if I were the engineer? And this intentionally biases me. But what it does is it brings me into the code review with critical thought, because now I can look for deltas between what I envisioned and what the developer actually did. and and not that i'm looking to just like minus one the commit or minus one the review because it doesn't match my thoughts not at all it's because now my brain is in like pattern recognition mode which is what brains are good at instead of just like well looks like you didn't indent this as well as you should have or we do tabs not spaces you know or whatever you know dumb things

instead i'm looking at structure and design and code organization things that actually matter much more than like hey we put curly braces on the same line as the f statement buddy sure clean that crap up clean that i won't have it our customers will be livid if they knew that we had curly braces yeah so two things that what you said made me think of the first is just at the very beginning you mentioned that people who are good at it put a lot of time into it and i think there's this idea in the industry that code reviews are like a quick fix you just do them slap them on and then they make your code better and i think that is untrue i think you're totally right that good code reviews it involves reading and understanding code and unless you

very intimately understand the chunk of code that's being changed already you kind of need to get up to speed on what the state of the code is before the change and then understand how the change affects it it takes a lot of time and uh i think recognizing that this is a thing you invest in. It's not just like a, yeah, like I said, it's not a quick fix, is key to getting value out of it. The other thing is, you mentioned those kind of little nitpicky curly brace things. There's a whole class of things that get caught in code reviews that you could automate away through your tooling. The curly brace thing, you should have some kind of linter or automated code formatter that just eliminates those issues. If you are spending time in code reviews looking for style

issues then that's time that computers can can save you from spending and then you have more time to spend on the the good stuff the functionality so i think that's totally worth investing some time into changing your ci setup or or your whatever your setup is for checking for those things to make sure it happens for you and you don't have to do anything absolutely well worth it um also that means you can have the discussion as a team about what the nitpicks are going to be once instead of every code review oh and beware of that discussion yeah but get it done oh just be done i hate those discussions no matter what everyone's always a little bit mad because they don't get all of their own way exactly i that's that's honestly probably the

thing that has made me the most mad in software yeah it's just arguing over stylistic things and i recognize in my head it doesn't matter jameson you're arguing over stupid stuff are you mad because you're watching two other people argue or are you mad no no i am actively participating because in javascript somebody wants to make us use triple equals all the time and i'm like that's stupid because sometimes double equals is better and if you just learn the rules for coercion then it makes sense and like and and i just i i see myself being unreasonable and i i have a hard time stopping it just like a runaway train mirror and you just feel shame i do yeah because i get you're a junkie jameson i am i am a junkie yeah i mean if everyone just adopted my style then

that'd be okay um well then you wouldn't be a junkie anymore you'd be the king the junkie king so when it comes to nitpicks you know for the stuff that your linter or automated ci system doesn't catch if you do have a nitpick that you notice and that you think is worth mentioning i like to tell people i'm reviewing code for hey here's a nitpick i will not withhold my plus one for this but i just wanted you to be aware it's something i noticed your call whether you fix it or not i don't think i really hate it when merge requests or pull requests get held up because there's five nitpicks and somebody is just digging in on it yeah yeah that makes sense oh there's one more thing back to the requester so one of the things you can do as a requester is you can give

the reviewer a sense for how finished this is for example if you are a work in progress it's like a third done and you just want to get a sanity check on your approach that's a very different review time commitment then this is a finished product and it must be ready to ship to production so telling the reviewer what state you're in before you give it to them i think is really helpful Yeah, that's a huge, that's a huge point. By huge, I mean good. I don't know why I said huge. The size of that point is larger than other points. Yeah. Even though the words were small, the point is large. And then the other thing is when you've got a work in progress, maybe you don't need a code

review at all. Maybe you need to go sit down with someone and talk. Code reviews incur a certain amount of communication overhead. They put one more item on someone's to-do list and they cause cognitive friction you know and sometimes when you sit down even though it might take a little more time you can have a better conversation that's less there's less cognitive load you know it's like hey i want to talk through an idea with you it's like way better than i've sent you a code review you know sure and so sometimes you don't need a code review at all you need a concept review and those do in my experience those are best done one-on-one or through some other means like a document that you can share yeah it's way easier to change a design than to change code

and true even the word design sometimes gets a bad rap it feels like a big design up front and that's bad and we were agile and we just kind of ship stuff and then figure it out and uh design is great and it can help make your code better and it's easier to do in your head than on the computer sometimes it really is all right that's that's all i have me too that question has been answered yeah it has your question has been reviewed plus one plus one looks good to me it seems fine i'm sure it's i'm sure our answers were fine i'm sure i don't understand most of them and i'm not going to try but it's fine i i think jameson you should put more pauses between your sentences just it's a nitpick but i just want you to know all right deleting the podcast starting

over oh i had one more thought on that oh hit me there's this really awesome episode on a podcast called The Change Log by the author of the C4, the Collective Code Construction Contract, Peter Hintjens, who, look him up on episode number 205 of The Change Log, great episode. One of the things he talks about in code reviews is he says, our protocol is we always move forward. So if a pull request comes up

and it's got some controversial stuff in it, we merge it, and if you care enough to fix the controversial stuff, you will make a subsequent pull request that addresses it if you don't care enough to actually make your own pull request to address the stuff that you raised then you don't care that much and just let it go and i thought that is a really cool heuristic i like that a lot too i in my younger days i've been guilty of arguing passionately for someone else to make something yeah make me feel better and then uh they were like well i'm not going to do that but if you

care you can change it and i didn't care enough and you didn't so you learned something about yourself i sure did yep i'm really glad i didn't know you back then same all right well uh i am i think we're out of time today oh that is a good point yeah we uh i think that was a good question and we got some good stuff in about the the fired stories we want to respect your time by not making it uh not making this podcast too long we want to respect your time by not making you listen to us very much yeah that sounds right plus one these are great questions if you have a question uh if our listeners have a question jameson what should they do um no wait wait wait that they want us to answer oh that they think that we

could answer about yeah soft skills and coding real oh geez i already messed that up not the not coding stuff the core premise of our podcast uh is clearly not cemented in my mind yet after 20 episodes anyways they should send us a tweet or a direct message on twitter we are soft skills eng we will put it in our backlog and we will uh try and get to it we love um hearing feedback about the episode too what you liked and what you didn't like uh if someone could make one of those inspirational quote poster things of stuff that dave says where it's like white text there's a picture of the sunset and the text is overlaid um that would also be good feedback too where are you going with this like what kind of stuff no just like those inspirational things that people

post on facebook except it'll say like plus one dave smith or something like that that's all yeah that kind of feedback would be helpful i think you finally identified the financial model for this podcast it's brilliant it's a good thing because those yacht payments aren't paying themselves oh speaking of that okay speaking of financial models if you are interested in sponsoring the podcast we are interested in talking to you um i think a lot of people are listening now actually uh literally several of of humans are listening we've extended from jameson's mom to also a couple of his cousins yeah yeah they're they're big fans uh and i think it could be a good way to um have us help you out and have you help us out so if you're interested

also send us a tweet or a dm also because i want this done i'm gonna um announce this we have a website it's at softskills.audio and if you go there now there's nothing but soon there will be something because i'm gonna work on it so it's done in time for this publication way to commit publicly very publicly we're gonna put in place an aggressive code review process for any changes you make to the website james that will surely help it get done faster you're smart you don't need a code review thanks for joining us today it was great having you and thank you so much for those that wrote in stories and questions they just keep piling up and we will get to them Yep. Talk to you later.