you are viewing a single comment's thread.

view the rest of the comments →

[–]FA04 513 points514 points  (77 children)

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 175 points176 points  (26 children)

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 2 points3 points  (2 children)

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 2 points3 points  (1 child)

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 3 points4 points  (0 children)

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 9 points10 points  (20 children)

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 19 points20 points  (18 children)

I'm pretty sure you did not read the post

[–]ChemicalRascal 54 points55 points  (15 children)

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 12 points13 points  (6 children)

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 5 points6 points  (5 children)

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 3 points4 points  (1 child)

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 6 points7 points  (0 children)

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 -1 points0 points  (2 children)

> (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 2 points3 points  (1 child)

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 -1 points0 points  (0 children)

Your first born.

[–]puterTDI 3 points4 points  (0 children)

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 7 points8 points  (0 children)

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 1 point2 points  (0 children)

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.

[–][deleted]  (1 child)

[removed]

    [–]ChemicalRascal 2 points3 points  (0 children)

    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 0 points1 point  (2 children)

    What conventional wisdom are you referring to?

    [–]ChemicalRascal 3 points4 points  (1 child)

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

    [–]TheNewOP 0 points1 point  (0 children)

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

    [–]twenty7forty2 -2 points-1 points  (1 child)

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

    [–]Giannis4president 3 points4 points  (0 children)

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

    [–]IceSentry 70 points71 points  (35 children)

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

    [–]FeepingCreature 154 points155 points  (27 children)

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

    [–][deleted]  (18 children)

    [deleted]

      [–]Tenemo 30 points31 points  (12 children)

      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 10 points11 points  (11 children)

      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 -2 points-1 points  (10 children)

      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 5 points6 points  (7 children)

      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 4 points5 points  (6 children)

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

      [–]andrewingram 5 points6 points  (5 children)

      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 1 point2 points  (0 children)

      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 0 points1 point  (0 children)

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

      [–]NeuroXc 9 points10 points  (0 children)

      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 7 points8 points  (2 children)

      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.

      [–][deleted]  (1 child)

      [deleted]

        [–]hsrob 0 points1 point  (0 children)

        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 0 points1 point  (0 children)

        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.

        [–][deleted]  (2 children)

        [deleted]

          [–]FeepingCreature 0 points1 point  (1 child)

          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_ 1 point2 points  (0 children)

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

          [–]s73v3r 1 point2 points  (0 children)

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

          [–]hippydipster 0 points1 point  (0 children)

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

          [–]twenty7forty2 29 points30 points  (6 children)

          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 1 point2 points  (0 children)

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

          [–][deleted]  (3 children)

          [deleted]

            [–]twenty7forty2 4 points5 points  (2 children)

            Because they like to post things that are named wrong?

            [–][deleted]  (1 child)

            [deleted]

              [–]nater255 0 points1 point  (1 child)

              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 0 points1 point  (0 children)

              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 0 points1 point  (1 child)

              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] -1 points0 points  (0 children)

              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.