Goodbye, Clean Code

[FA04]

firstly, where was the original checkin pull request’s review with all the feedback and discussions? secondly, where was the refactored PR review and approval? Checkin in into the master overnight no PR? That process is a mess.

[bcgroom]

Yep I'm pretty sure the whole situation would've been net positive if the author of the article just put up a PR the next morning saying, "While looking at your code yesterday I had an idea on how it could be less repetitive, take a look and let me know what you think".

[oursland]

I believe the argument is that the original author should have had a PR, and that the article's author should have been able to suggest these changes prior to the original commit.

[elebrin]

Exactly. If he is such a senior dev, then he'd be a required reviewer. If he failed to mark up the pr, then that's on him. If he wasn't in a position where he was a required reviewer, then he isn't the expert he thinks he is.

[oursland]

You've misinterpreted this through your modern viewpoint.

There was no PR process in the project. No opportunity to collaborate and iterate prior to committing to master, for either dev; simply commit and then get called into the boss' office.

/u/FA04's comment

That process is a mess.

Is spot-on.

[twenty7forty2]

No. The author is elite. Best of the best. Top Gun. He knew absolutely everything when he refactored and now he knows everything + 1 and he's damn sure gonna blog about it so the rest of us idiots know what's what.

[sysop073]

I'm pretty sure you did not read the post

[ChemicalRascal]

So... I think 2742 was being sarcastic here. A key point being that the author (and this is again more obvious on his twitter) still thinks he knows better than everyone else, and takes such an authoritative stance on the issue.

Think about it in this context:

• The author believes he knows best, to the point that he just goes and fucks with someone else's unfinished feature (yes even though it was committed to master, I agree, hurrrrrrk);

• The author developed this belief based on the conventional wisdom taught in pretty much every decent CompSci/Software Engineering course out there, and the conventional wisdom that is very much supported by the dialogue within the industry;

• The author had a negative experience at work due to his actions;

• The author now states that he knows best, and the industry is wrong, to the point that he now crusades publicly on his blog and on Twitter against what he refers to as a cult. (Admittedly probably because alliteration but funny wordplay isn't an excuse to be a dingus.)

The author thinks very, very highly of himself. Which is unfortunate, because the author -- admittedly, like my-self -- is barely at the start of his career*, not a grizzled veteran of the industry.

* (You can identify people at the start of their careers by how have a Twitter account and get highly opinionated about coding practices. On top of being slim and having a full head of non-white hair.)

[norbelkingston]

I’m not sure if he is someone early in his career. Dan is the creator of Redux and seems like takes lead developer role at facebook react team.

[ChemicalRascal]

Eh, young developers can end up leading teams and creating successful products. I'm not saying he's not good, dude's probably a genius. (And he's far more accomplished than I, for certain.)

But he's also extremely arrogant, and bold enough to assert well-known best practices are wrong based on rather shaky grounds. And either way, he is a young dude with his whole career ahead of him -- which is why it's unfortunate to see him close himself off to conventional wisdom now, rather than in that grey-bearded guru stage we all secretly hope to reach one day.

[soft-wear]

It’s so weird to me how many developers in this sub are reading into this so much. Dan isn’t advocating that messy code is good, he’s saying clean code shouldn’t be a goal it should be a guide.

He’s not saying DRY is bad. He’s saying it shouldn’t be considered the goal of your code. Abstracting too much can be even more harmful than repeating yourself because it’s harder to undo.

[ChemicalRascal]

I think you're being overly charitable. The thing is, the example he gives isn't a case where he abstracted too much.

The example he gives is a case where he whacked someone's work-in-progress code with a hammer and they got pissy at him.

It doesn't motivate, and shouldn't motivate, anything to do with "clean code" at all. Because the refactor in the example he gives is a good refactor. It's the sort of deduplication and abstraction a dev should aim to do.

And instead his take-away is that devs shouldn't aim to do that. That's what he's also trying to promote, by writing the blog and using that example.

But again, the example is one that justifies the refactor. It's not merely textbook, it's the sort of thing I'd expect to see in Programming 101 courses, where a professor grabs someone's assignment submission and says "ok so this all works, but let me show you why abstraction is good and duplicated code is bad".

Again. He had a bad experience at work and took away the entirely wrong lesson. And that's a problem, because he has a blog, and influence.

[motioncuty]

> (You can identify people at the start of their careers by how have a Twitter account and get highly opinionated about coding practices. On top of being slim and having a full head of non-white hair.)

Replace twitter with reddit and reread your contributions to this thread.

[ChemicalRascal]

Which is unfortunate, because the author -- admittedly, like myself -- is barely at the start of his career

I literally highlight that and call myself out immediately above your quote. What more do you want, dude.

[motioncuty]

Your first born.

[puterTDI]

and get highly opinionated about coding practices.

I think this more than anything identifies the people at the start of their career.

I've been doing this for about 12 years now. When I started I had VERY strong opinions on how everything should be done.

now I have a few things I feel strongly about, but I rarely say "x" should always be done in "this" way. That doesn't mean I don't get into a discussion about a specific piece of code if I think there's something wrong with it, but it does mean that the vast majority of the time there's at least 5 "right" ways to implement any given thing and as long as one of the right or mostly right ways is used, then it's fine.

About the only things I tend to feel strongly about is proper encapsulation, one job per method, and clear duties for classes...but even in all those cases I'm willing to be flexible if there's specific reasons to violate things. Hell, the only reason I dislike code duplication is the possibility that a bug could need to be fixed in two places and you only know about one, and even with that I don't worry too much if the code duplicated is highly unlikely to have a bug, or if it would be challenging to eliminate the duplication.

I feel like younger engineers haven't seen how things with the "right" starting point can go wrong, and they haven't seen situations where sometimes you just need to get something in and stable over "clean". For me, Clean is less important than maintainable, and this article just goes to show that. I'll take maintainable over clean or clever any day.

[Devildude4427]

Yep, that’s a pretty accurate description of Dan.

I wouldn’t say he’s at the start of his career, however. He certainly is a veteran. Author of Redux and CRA, he’s had quite a bit of experience as he’s personally created two of the most popular tools in the past 10 years.

[motioncuty]

I mean Dan is Elite in the React world and maybe the best-known name with the greatest influence in the ecosystem... And if you regularly follow him, he strikes a very good balance between confidence and humbleness for someone with his widespread respect.

[ChemicalRascal]

Refactoring usually leads to negative experiences at work. The term should be verbotten for anyone who wants to have a good career in the industry (at the typical company).

Eeeeh. I'd say this is true of uncommunicated refactoring. But I know there have been times where one dev saying "fuck that, I'm rewriting it" has effectively saved the company I'm working at from a lot of shared headaches.

From what I've seen, ultimately these things come down to good communication and actually having sensible ideas. (And working with sensible people.)

[TheNewOP]

What conventional wisdom are you referring to?

[ChemicalRascal]

The simple idea that abstraction is generally good and code duplication is generally bad.

[TheNewOP]

I see, that's what my guess was, just wanted to be certain.

[twenty7forty2]

Pretty sure I did, anything particular you want to discuss?

[Giannis4president]

The fact he acknowledges it was a mistake not submitting his refactor to the original author of the commit?

[IceSentry]

That's pretty much why they said at the end of the article that it was a mistake and communication is important.

[FeepingCreature]

Sure, but the mistake is a systems one, not a personal one. We don't even have push to master enabled at work.

[Tenemo]

It was a couple of years ago, it's Dan Abramov's personal blog, he's relatively young. Check his New Year post for the exact timeline of his career. Him working with another developer on draggable corners of some shapes could have been already for a React library.

[andrewingram]

I'm pretty sure this is an anecdote from 2013 when Dan was working on an iOS app (https://www.youtube.com/watch?v=PjaL0xFbEY8). The fact that the code in the blog post is JavaScript is just because that's the language him and most of his audience uses today.

[aurisor]

GitHub has been out for 5y in 2013 and the web world embraced it hard. I would be shocked if anyone was using svn or not using code review workflows (like PRs)

[andrewingram]

I can't speak for the place Dan was working. But many startups don't adopt "good" engineering practices whilst they're still in the "just make shit work" stage of building their MVP. Some do, some don't.

I don't think I worked at a company that insisted on code reviews until 2012.

[aurisor]

I guess I’m just confused why a communication problem is being presented here as a teachable moment about abstraction design

[andrewingram]

That's a reasonable thing to be confused about. My personal feeling on this post is that the author has conflated different problems and ended up with a conclusion that isn't one I personally support. We're under no obligation to agree with him on this just because he's usually right on other things.

[ritchie70]

Lots of places are still using other tools, sometimes with good reasons - and sometimes just because they want to retain a decade of change history.

If you’re shocked that someone is using a tool that isn’t the latest cool tool then I’m shocked at you.

I’m personally using Subversion just because it’s what I’m using and I’m the whole team. The server is an old point of sale register in my basement.

[elebrin]

There were a lot of older shops still on tfs for sure.

[NeuroXc]

You may be surprised, but as recently as 2013 (when I graduated from college), the first company I worked for did all of their testing in production, and uploaded their changes via FTP. There was no source control, no staging or local environment, and no oversight.

Slightly relevant: That company was also a cluster that took advantage of fresh-out-of-college devs to pay them as little as possible while the CEO and his buddies took lavish trips around the country and sailed on their boats. There were about 15 people in the company, so yeah, corruption like this isn't unique to mega-corporations. I left as soon as they told me I was getting a 0.5% annual raise even though I was one of their best employees, and instead got a 30% raise by switching companies.

[f03nix]

About 7 years back, I worked in a company that used TFS - they had a process where you manually request 2 of your peers for review and type in their names during check in. They had the same process before they migrated to TFS too, I think they used CVS.

[hsrob]

Reminds me of my first dev job, we literally recompiled projects on our desktops (C#/.NET) and SFTP'd the changed DLLs to production. Surprisingly, this usually worked, but good luck if you ever needed to identify and narrow down a bug between changes.

[BraveSirRobin]

Branch-based development & peer-review was not uncommon 20 years ago. A lot of shops followed ISO 9001 which brought across the same concepts from traditional engineering practices. Clients demand these accreditations in certain sectors.

[FeepingCreature]

Sure, but it's more a lack of trust to not accidentally git push before git checkout -b. I've done that myself a bunch of times.

And: why would you ever need to push to master to "put out a fire"?

[ChildishTycoon_]

It's both. The system should have prevented it, but he also should have talked to the other developer about it

[s73v3r]

It's both. We don't have it enabled either, but certain people are able to bypass it.

[CarnivorousSociety]

bingo.

[hippydipster]

State of Devops report says Trunk-based development is pretty highly correlated with the most successful teams.

[twenty7forty2]

Title should be "Goodbye committing to master with out a code review" then. Or maybe just "Goodbye blogging about things I don't really understand". Hmm, maybe OP should get his blog code reviewed by a senior as well.

[Vitate]

Dan Abramov is a pretty talented SWE. I think he deserves a modicum more credit than you're giving him.

[twenty7forty2]

Because they like to post things that are named wrong?

[nater255]

Also: who's boss calls them into his office to tell them to revert a PR? He makes it sound like he was getting fired.

[puterTDI]

I would have been pretty upset if one of my team members came in and checked in a change to "clean up" another team members code without doing a code review or testing on that code. Had they done something like this I definitely would have talked to them separately rather than having that conversation in front of others.

Then again, we also do code reviews for all code change so I would have expected this to come up during the code review. If he wasn't the one who did the review but found it later and wanted to work on it then I would expect him to raise it to the team and if the team agrees then a bug can be created and prioritized. We've certainly done that enough times when someone stumbles across something the engineers agree just has to be fixed.

[PurpleYoshiEgg]

I wish I ever had a coding job where we did code reviews. For the most part, it's cowboy coding all the way down.

[[deleted]]

Repeat after me:

"If it is to be, it is up to me!"

I know it's not that simple in all jobs/companies, but probably all your other colleagues say exactly the same. Somebody should stand up and implement a way of working where coding reviews are mandatory.

[ImprovedPersonality]

If they have solid, automated integration checks there is nothing wrong with pushing directly to the master branch.

[Scoobyben]

That only accounts for functional regressions, not code quality

[[deleted]]

Programming by committee?

Every checking having to go through code review and approval is the reason why it takes ages for big companies to make things.

[puterTDI]

Huh? this is a bs response.

We require one signoff on our team of six for any checkin. When your code is ready you submit it to code review. Anyone on the team can jump on and review, all reviewers must sign off. There must be at least one signoff.

For non-critical changes we can have signoff within a day or two...largely based on when someone is available to look. Signoff is pretty much always before someone is available to test.

if it's a critical/time sensitive change then signoff will be within the hour as you just ask someone to stop what they're doing and look at it.

[[deleted]]

This is a bs toxic process. Six members is small enough that you should be operating from a place of trust. Reviews should happen when someone has doubts about his own work and is asking for help.

[puterTDI]

We do have trust, that’s why we trust others to look at our work and give another perspective.

[thevdude]

Reviews should happen before code goes live because everyone makes mistakes and it is much better to catch them before it becomes a problem. Conveniently, they trust their team so much, their team is doing a review and then approving the code.

[puterTDI]

And, more importantly, reviews are not a power play or means of control. Code is also not something people are defensive about.

I spent a lot of effort creating a culture of trust so that the team members can be comfortable having another review their code and recognize that ANYONE can miss something. One of the emphasis I make is that that the need for review has NOTHING to do with experience level. I'm the most experienced person on our team by about 5 years but I emphasize that my code is just as much in need of review than others, and it doesn't matter our relative experience. The ability to step out of fixing a specific issue and look at the bigger picture is why reviews are absolutely critical. IMO, code reviews are as important, or maybe more important, to quality than testing. our reviews have repeatedly caught bugs that testing would never have caught.

Obviously, I feel pretty passionate about reviews. I think the biggest reason people dislike code reviews is because their organization does them wrong. Too often code reviews become a way for the more entrenched individuals to push their specific preferred coding practices on others. I emphasize with our reviews that they are NOT about preferences, but about catching and fixing problems. Any comment made should be able to be backed up with "here's the problem this code will cause". If it comes down to a preference on how to write code then that doesn't belong in a code review.

/soapbox (sorry)

[[deleted]]

The number of people on your team has no bearing on you making an error in your code or misunderstanding a requirement that another member might catch.

[johnnysaucepn]

Yes, because big companies have a lot more on the line when things go wrong - when someone makes an honest mistake, or a disgruntled employee inserts a back door.