[HN Gopher] Goodbye, Clean Code
___________________________________________________________________
Goodbye, Clean Code
Author : danabramov
Score : 191 points
Date : 2020-01-11 21:32 UTC (1 hours ago)
(HTM) web link (overreacted.io)
(TXT) w3m dump (overreacted.io)
| 29athrowaway wrote:
| > Firstly, I didn't talk to the person who wrote it. I rewrote
| the code and checked it in without their input.
|
| If you want to modify a method that has 10 unique contributors.
| Do you really need to talk to different 10 people to maintain to
| make a change? That does not sound very effective.
|
| And, most importantly: when you code as a job, all your
| deliverables are company's property. They are not yours. The
| company can do whatever they want with them, they don't need your
| opinion or approval. If they want to replace every piece of code
| that you ever submitted with an ASCII clown, then print the
| source code make a pinata from it, they can do that too if they
| want.
|
| Secondly: version control. If you want the old version of some
| code, you can retrieve from version control right? You can leave
| a comment such as: "for a verbose version of this method, see
| revision <sha1>".
| yoz-y wrote:
| > If you want to modify a method that has 10 unique
| contributors. Do you really need to talk to different 10 people
| to maintain to make a change?
|
| I think the more apt example in this case would be if somebody
| made a modification in that file and you rewrote just their
| modification immediately after they done it without discussion.
| If the modification is necessary so soon it means that the
| feature/implementation were not discussed properly before and
| the process probably needs revisiting.
|
| Of course if you are modifying part of the file, or doing some
| previously agreed refactoring effort then explicit author
| consent is not necessary.
|
| > Secondly: version control. If you want the old version of
| some code
|
| I think here we talk mostly about production code. The problem
| with relying on the source control for "I will revert this to
| the old version when it will become required" is that the code
| will evolve in subtle manners and it will be hard to roll back
| and apply all patches on top of it.
| flurdy wrote:
| > > Firstly, I didn't talk to the person who wrote it. I
| rewrote the code and checked it in without their input.
|
| > Do you really need to talk to different 10 people to maintain
| to make a change?
|
| No, you don't talk to all of the 10 contributors. But if you
| want to still work as a team you should talk to a few.
| Depending on the size of the change.
|
| Some or even all of the previous contributors may no longer be
| at the company or are inaccessible. But a super quick
| discussion with 1-3 other team members should float if this is
| a good idea or not.
|
| Obviously, if you practise pair programming, most smaller
| rewrites just need a consensus within the pair, and a more
| extensive change may be a good idea to get the approval of
| another pair, especially if some other team members were the
| original contributors.
|
| There is no need to be precious about any existing code. Nor
| any need to be a bull-in-china-shop either.
| 29athrowaway wrote:
| For quick changes, you can simply submit your changes in a
| pull request and assign them as reviewers.
|
| For non-quick changes (that are worth the cost), you can have
| a discussion about it and evaluate pros and cons.
| awinter-py wrote:
| IMO line count is an important metric
|
| if two companies are broadly similar but one gets the job done
| with 10x less code, they're probably in a better survival
| position
|
| not all logic _should_ be reusable or super generic, but there 's
| a size level at which avoiding abstractions becomes toxic to
| further growth
|
| there's a good article floating around somewhere about things you
| learn at 1k, 10k, 100k-line codebases and how your philosophy
| changes
|
| with a million caveats, of course, size and maintainability are
| going to be correlated
| bfung wrote:
| Did the colleagues code go through code review before it was
| accepted?
|
| And obviously, there wasn't a review for the refactored code
| before check-in.
|
| Use a code review system, would e avoided a lot of this in the
| first place.
| Dirak wrote:
| I would even take your learnings from your experience a step
| further and say that one should be proactive in talking to
| teammates and exploring ideas on how to improve the codebase if
| you think there is room for improvement. For instance, it may
| well be that inheritance is the wrong pattern for that given
| codebase, but there are indications that the current architecture
| isn't scalable.
|
| With more investment, more research could have been put into
| looking at better suited architectures such as the Entity
| Component System architecture, where Entities such as Rects and
| Ellipsis are composed of units of functionality (position,
| resizability, etc), which is a proven architecture for this sort
| of application. Then, implementation would be a matter of getting
| team buy-in / weighing the refactor pros and cons with your team.
|
| Tangential, but Entity Component Systems are a great way to
| structure systems with many types of entities that share subsets
| of functionalities. https://kyren.github.io/2018/09/14/rustconf-
| talk.html.
| Havoc wrote:
| I feel the code aspect could have gone either way.
|
| It's the human part that went sideways. Egos are fragile & this
| is essentially "your code is bad and I can do it better".
|
| I've been on the receiving side of this exact thing - also
| duplication. Fortunately the dup was bad enough that I could see
| I was wrong.
| kevin1024 wrote:
| Duplication is far cheaper than the wrong abstraction.
|
| https://www.sandimetz.com/blog/2016/1/20/the-wrong-abstracti...
| jrs95 wrote:
| To me the #1 thing that fixes the social issues described in this
| post (changing code someone checking in code a few hours after
| they pushed it) is a code review process. The most valuable thing
| about always doing pull requests and code review is sharing
| information and having discussion about changes, not necessarily
| the immediate improvement of the code. A team will gain more
| knowledge about what everyone is doing by reviewing each others
| pull requests than they do from a daily standup by a pretty large
| margin in my opinion.
|
| In this particular example, reviewing a PR would have meant that
| a discussion about whether or not that abstraction was a good
| change _before_ the code was ever merged.
| BeetleB wrote:
| The first thing I did when I read this was come to the HN
| comments and search for people complaining about DRY:
|
| https://news.ycombinator.com/item?id=22022599
|
| https://news.ycombinator.com/item?id=22022842
|
| https://news.ycombinator.com/item?id=22022896
|
| https://news.ycombinator.com/item?id=22022836
|
| The author of this piece was not engaging in a DRY activity even
| if he thought he was. He (perhaps unwittingly) admits to it
| himself:
|
| > My code traded the ability to change requirements for reduced
| duplication, and it was not a good trade.
|
| The acronym DRY was coined in _The Pragmatic Programmer_ , and
| the author of that book makes it clear that DRY is about not
| repeating _requirements_ in code. You definitely don 't want to
| deduplicate similar code if it involves _multiple_ requirements
| because then you entangle yourself, which is what the author of
| this piece did.
| monksy wrote:
| Are you saying that they're repeating themselves?
| monksy wrote:
| To me this sounds like he gave up on producing something with
| craftsmanship. It sounds like he's getting bullied by someone of
| lessor experience.
|
| If the code was blainently duplicated, that's going to create
| problems later.
| jcims wrote:
| Isn't the only reason we care about duplicate code that we don't
| want to miss an instance of it for enhancements or bug fixes down
| the line? Don't we have technology that helps us reduce that
| concern?
| Rapzid wrote:
| Yeah, it's not about text/code duplication as much as it's
| about behavior duplication. In theory both, hopefully three or
| more!, code paths share the exact same behavior and if that
| behavior ever changes it will need to change EVERYWHERE you
| introduced a method to abstract it away.
|
| Depending though, the indirection trade off may not be worth
| the abstraction to DRY it up. Then you end up with code like
| what was popular in the bad old days of Ruby. Essentially, you
| can get carried away.
| sirtoffski wrote:
| Off topic: Docusaurus V2 makes a really clean looking blog via
| GitHub pages. Cheers
| misterCrowley wrote:
| Clean code and Agile is excellent and is important to use in
| enterprise applications. The author needs to educate himself
| smacktoward wrote:
| My rule is: if I find myself sorting things into two buckets, one
| of which I've given a name that obviously means "good" and the
| other "bad," that's a sign I'm deciding emotionally rather than
| rationally. It's time to take a step back and think about the
| distinctions between the things in the buckets harder.
|
| Sorting code into "clean" and "dirty" buckets is a good example
| of this. Both bucket names are completely subjective, with
| "clean" obviously meaning good and "dirty" bad. As the article
| indicates, dig in a little deeper and it's not hard to find
| objective ways the "dirty" code would actually be preferable.
| quaquaqua1 wrote:
| what the fuck did i just read
| rckclmbr wrote:
| A good post about someone learning that writing code is more
| than just about how pretty it looks. How you work with others
| is incredibly important in this industry
| BelleOfTheBall wrote:
| Is that not common knowledge? I feel like this is a well-
| written post with a good point but it's a familiar point. You
| could boil part of it down to 'all's good in moderation', so
| don't just keep your code clean, keep it clean and easy to
| read, etc.
| dictum wrote:
| Leaving common knowledge undocumented is, in itself, a
| misguided attempt at DRY.
|
| Your common knowledge isn't necessarily everyone's common
| knowledge; what you perceive isn't necessarily _what it
| is_.
|
| Every day there are many people just starting to code.
| There are cultural and regional differences. They might not
| have hit the exact spot in which this information appears.
|
| Knowledge also disappears, fades, gets censored or
| destroyed, gets mixed up and remembered incorrectly. You
| might take something as a given and misremember it 20 years
| from now. You might read your own code 5 years from now and
| not know _why_ something 's there -- something that was
| taken to be obvious.
| mannykannot wrote:
| It definitely is something that needs to be said, as it is
| not uncommon to find people doing the opposite: taking a
| rule-of-thumb and asserting that it is the one true way.
| The people who are most likely to make this mistake are
| smart and well-motivated, enthusiastic about the power of
| abstraction to simplify things, and have some experience
| but not a lot.
| danabramov wrote:
| I didn't mean to imply there's anything novel in my post
| :-) someone's gotta beat that drum once in a while.
| vga805 wrote:
| Thank you Dan, your posts have been a great source for a
| lot of us out here.
| adamisom wrote:
| Would it help to point out the the author of that post invented
| React?
| azangru wrote:
| Would it help to point out that he didn't? :-)
| danabramov wrote:
| Thanks but I'm only helping maintain it. :-) I did co-author
| a few other projects though.
| tudorpavel wrote:
| Not really and Dan didn't invent React, he is currently on
| the core team. Just ignore trolls such as GP, it ain't worth
| it.
| [deleted]
| Insanity wrote:
| I agree that not every "smart approach" is worth it if you
| sacrifice legibility.
|
| But I don't think you necessarily need to ask permission to
| refactor code. On the projects I had the past couple of years
| everyone understood that code was open to be changed by anyone.
|
| In practice we'd often ask "why did you do X" instead of just
| rewriting it.
|
| I trusted the people I worked with on those projects though and
| if they thought they had to rewrite part of my code for whatever
| reason, that was fine by me. And vice versa, I changed their code
| and that was fine.
| monksy wrote:
| That and follow it up on a code review pointing out that it's
| duplication that could lead to bugs later.
| scarface74 wrote:
| Well in his case, did refactoring the code to clean it up add
| business value?
|
| There has to be a very good reason for me to refactor
| existing/working code for instance for performance.
|
| I won't refactor code to reduce existing duplication but I will
| refactor code if I see there is some functionality that I need
| elsewhere so I won't just copy and paste.
| Nullabillity wrote:
| > Well in his case, did refactoring the code to clean it up
| add business value?
|
| Yes, it made future modifications easier and discouraged
| adding special-case behaviour.
|
| And after reverting the change they did apparently fall into
| that trap.
| franciscop wrote:
| This should rightfully be on the front-page of HN. I've gone
| through something almost the same as Dan Abramov here, and I feel
| it's difficult to impossible to explain this and you have to
| experience it yourself.
|
| The tricky bit is where to draw the line, which I've learned only
| after pouring thousands of lines of code and I understand is a
| bit different for everybody. The rule of three[1] was a very
| useful rule of thumb in the beginning, but there are many
| secondary variables that I unconsciously use to decide when to
| remove repetition. e.g.:
|
| - Single file tends to be split _less often_ , since refactoring
| it is easier. Example: the single file routing in React being
| repetitive is okay, but if there's multiple files with routers
| and custom logic I'd consider a helper a lot stronger.
|
| - Conceptually straightforward APIs tend to be split _more often_
| , since it's easy to package and reason about, as well as design.
| Examples: cookies, warnings, kv stores, etc.
|
| - Early stage projects tend to be split _less often_ , since few
| things are not yet clear and being able to put everything in your
| head is a lot more important.
|
| It's also one of the lessons that you should learn going to
| senior. To the extreme, someone insisting in removing this kind
| of duplication for the sake of it is often a sign of a junior dev
| (as in, because it's _wrong_ and not trying to understand the
| codebase /tradeoffs first).
|
| [1] https://blog.codinghorror.com/rule-of-three/
| acjohnson55 wrote:
| I find myself wishing the question of whether to factor code out
| wasn't binary. Like if you could transclude parameterized code.
| You'd get the benefits of not having to keep a stack trace in
| your head, while still having a canonical version of an
| abstraction. Like a macro, but always expanded in-place.
| paulnechifor wrote:
| Sorry for the off topic grammar question, but am I the only one
| who finds it confusing how people have started to use plural
| pronouns to refer to individual people?
| jdashg wrote:
| It started in the 14th century. Wikipedia has a good article on
| it: https://en.m.wikipedia.org/wiki/Singular_they
| ChrisMarshallNY wrote:
| I'm finding this kind of issue coming up a lot with the current
| abhorrence with polymorphism and inheritance.
|
| I like using interfaces and protocols, but I also still very much
| use inheritance. It's a fundamental tool that was invented for a
| reason, and, sometimes, it is the best tool for the task.
|
| I've probably weathered just about every "paradigm shift" that
| has happened in software development. At one time, using
| variables with names longer than four characters was considered
| bad programming.
|
| Anyone remember GOTO?
|
| Some older constructs (like the two above): good riddance.
| Others...not so much. Structured Programming, which was declared
| The Mark of Satan, at one time, is still very much the basis for
| all our work.
|
| I love a lot of the new tools and techniques, but I still mix in
| a lot of the older stuff when I write software. To some, this is
| "unclean," because it doesn't tick some arbitrary "buzzwurd _du
| jour_. "
|
| Simple, solid code is always a great starting place.
|
| The author talks about removing repetitiveness (DRY). I think
| that's excellent, but, in my experience, I need to be very, very
| careful when I do that, as the original author may have tweaked
| just one little line, in one of the clones, and my refactoring
| may break things; sometimes, not until it's been out to the
| customers for six months.
|
| That's pretty much _de rigueur_ for any refactoring; not just
| DRYdock. In my experience, having some robust unit tests and test
| harnesses in place is absolutely required.
|
| I tend to write code iteratively. I'll start with some naive,
| sloppy code that works; maybe not well, then refactor it in
| stages, testing the heck out of it; each time.
|
| I used to work for a Japanese company. I had many differences
| with my Japanese peers, but they were the most disciplined
| programmers I've ever encountered. Whenever they would modify
| code, they would leave the old code in there, but commented out,
| and add some comments, explaining what their new code does.
|
| Made for some pretty verbose source files, but it was immediately
| apparent what was done, and why (I think the practice began
| before most good VCSes were invented). It also gave you the
| original code to copy and paste, if necessary. Very old-
| fashioned, but it made their changes (and bugs, therein), easy to
| understand. It also helped because the code was often stepped on
| by _many_ programmers.
|
| I'm thinking that a lot of folks are relying on commit comments
| to explain changes; which is good, but adds extra time to
| figuring something out.
| solidist wrote:
| AHA
|
| https://kentcdodds.com/blog/aha-programming
| Traster wrote:
| I think this is a great post, it covers a topic succinctly and
| agree with its conclusion.
|
| On the topic of actually refactoring code I think we should
| consider the code as a variable - that is to say, sometimes these
| variables just happen to equal each other in which case they are
| two separate things. Sometimes two variables aren't just equal,
| but they're the same. That's when to factor out the code.
| Otherwise the second these two variables are no longer equal you
| end up in trouble. The secret is divining when something is _for
| all intents and purposes_ the same as something else in this
| specific situation.
| boffinism wrote:
| I think this highlights an important nuance to the Don't Repeat
| Yourself maxim. It's not just about whether two bits of code are
| similar now. It's about whether, when they change in future, they
| are likely to change in the same ways. If not, then DRYing up the
| code now is only making more work for yourself down the line.
| tudorpavel wrote:
| DRY is good but like any abstraction it has tradeoffs. It might
| give you a warm feeling to do the refactor, but you might end
| up with code that's harder to understand and harder to replace.
| erik_seaberg wrote:
| I've always found the
| https://en.wikipedia.org/wiki/Open%E2%80%93closed_principle,
| where you derive from classes instead of editing them in-place,
| sort of appealing for this reason.
| terandle wrote:
| What is this implying? That instead of adding a
| property/method to a class you should instead create a child
| class that inherits from the original with the new
| properties? Sounds like a great way to have a very
| complicated class hierarchy IMO
| erik_seaberg wrote:
| Exactly that. Just because _you_ want new behavior doesn 't
| mean all the _other_ consumers of that class also want it.
| The old behavior should still exist, and have a name. I 'm
| not sure whether the idea was ever fully developed into
| patterns for replacing deep hierarchies with simplified
| classes that factor out some inheritance from otherwise-
| unused base classes.
| burlesona wrote:
| I've usually heard this phenomenon called "incidental
| duplication," and it's something I find myself teaching junior
| engineers about quite often.
|
| There are a lot of situations where 3-5 lines of many methods
| follow basically the same pattern, and it can be aggravating to
| look at. "Don't repeat yourself!" Right?
|
| So you try to extract that boilerplate into a method, and it's
| fine until the very next change. Then you need to start passing
| options and configuration into your helper method... and before
| long your helper method is extremely difficult to reason about,
| because it's actually handling a dozen cases that are
| superficially similar but full of important differences in the
| details.
|
| I encourage my devs to follow a rule of thumb: don't extract
| repetitive code right away, try and build the feature you're
| working on with the duplication in place first. Let the code go
| through a few evolutions and waves of change. Then one of two
| things are likely to happen:
|
| (1) you find that the code doesn't look so repetitive anymore,
|
| or, (2) you hit a bug where you needed to make the same change to
| the boilerplate in six places and you missed one.
|
| In scenario 1, you can sigh and say "yeah it turned out to be
| incidental duplication, it's not bothering me anymore." In
| scenario 2, it's probably time for a careful refactoring to pull
| out the bits that have proven to be identical (and, importantly,
| _must be_ identical across all of the instances of the code).
| lilyball wrote:
| I have 2 rules I use when determining whether to duplicate code
| or to refactor:
|
| 1. How many duplications are there? If the code is duplicated
| once, that's fine. If it's duplicated twice (so 3 instances of
| it), then it's time to consider refactoring, subject to the
| next rule.
|
| 2. Why is the code duplicated? If it's "incidental
| duplication", i.e. code that happens to look the same, don't
| refactor. Only refactor if there's actually a reason why the
| code is the same. Which is to say, attempt to predict the
| future: if a change is made to one instance of the code, do I
| expect it to be replicated to the other instances too?
| scarface74 wrote:
| That can be boiled down to the "Rule of 3".
|
| My CTO often asks me to implement a feature to do X and make it
| "generic enough to handle future use cases". My answer is
| always the same - either give me at least three use cases now
| or I am going to make it work with this one use case. If we
| have another client that needs the feature in the future then
| we will revisit it.
|
| Of course, there are some features that we know in advance
| based on the industry how we can genericize it.
| [deleted]
| rdiddly wrote:
| "Premature optimization something something..." ;)
| RealDinosaur wrote:
| I think the catch all term for that is YAGNI.
| specialist wrote:
| After witnessing the collateral damage of many software reuse
| projects (anyone remember "components"?), I came up with a
| different ruleset, useful for "compromising" with "software
| architects":
|
| First, translate the data.
|
| Second, divine a common format and share the data.
|
| Third, create the libraries for this common format, to be
| reused amongst projects.
|
| I have never reached #3 in my professional career. Sure, we
| wrote the libraries. But other teams, projects have never
| adopted before whole effort became moot.
|
| So I kept my projects in tact and moving forward, while
| letting mgmt think they're doing something useful.
| daxfohl wrote:
| I call these "spaghetti abstractions", and they're the worst
| kind of spaghetti.
| yodsanklai wrote:
| > Then you need to start passing options and configuration into
| your helper method... and before long your helper method is
| extremely difficult to reason about
|
| In which case, you should split the helper function ( extract
| sub-part common to all cases, and report the differences where
| the helper function is called).
|
| I think I would most of the time go with de-duplicating as
| early as possible, as long as the helper function only has few
| parameters and can be described in plain english rather easily.
|
| To me the cost of having to refactor the helper function later
| in the process is less than dealing with duplicated code.
|
| Duplicated code causes many issues. You mentioned introducing
| bugs, but it also makes the code harder to read. Every person
| who reads the code has to make the de-duplication effort
| mentally (check that the duplicated parts are indeed
| duplicated, figure out what they do, and on what parameters
| they differ...).
| specialist wrote:
| Upvoted. We'd probably come up with different
| implementations, I strongly prefer composition and
| "interpreter" style code, but whenever I've seen casual
| mutations in different dataflows, it was because of doing one
| off changes while ignoring the whole.
| barrkel wrote:
| Premature optimization. Duplicated code is only evil when
| there's a bug you only fix in one place and forget about the
| duplicates; in almost every other case, it's easier to reason
| about and is more resilient in the face of local changes.
|
| Abstraction is often like compression, and compressed data is
| easier to corrupt. Change the implementation of an
| abstraction, and you put all consumers of it at risk. It's
| not an absolute good.
| yodsanklai wrote:
| > it's easier to reason about
|
| Consider you have 4 times a block of 10 lines of code, they
| are identical except for a couple of parameters. The person
| who reads the code has to 1. figure out what the code does
| 2. see if the duplicated parts differ in some subtle way.
|
| The alternative is to replace the duplicated parts with a
| function that has a meaningful name. This makes the code
| easier to read. It's not a premature optimization. It would
| make sense to keep it duplicated if you're in an
| explorative phase and not sure yet what the final design
| will be, but I wouldn't submit a patch where parts are
| obviously similar. I'm not sure it would pass review.
| mfateev wrote:
| Go error handling is a good example of this. So far all the
| attempts to reduce the repetitive `if err != nil { ... }`
| through some abstraction failed. Look at
| https://github.com/golang/go/issues/32825
| renox wrote:
| In the language maybe but both Rust and Zig show that it's
| possible to have much less 'bloat' for error handling even
| without using exceptions.
|
| I'd say that go designers have still work to do: Zig
| especially show that you can be a 'simple' language and yet
| have both sane error handling and generics.
| m0zg wrote:
| And a rule for Java programmers as well: nobody is going to
| "extend" your program. If your interface has only one
| implementation, you do not need the interface at that time, and
| possibly ever. Nor do you need DI or whatever other
| masturbation "best practices" Java gurus prescribe. When it
| comes down to it, Java is a simple, pleasant language. So
| people invent all sorts of indirection to appear smart. Don't.
| Just do it simply and readably, with as little indirection and
| abstraction as possible.
| detaro wrote:
| And in the other direction, when adding flags etc to a method
| that was created to reduce duplication, consider if splitting
| it into two "duplicate" copies or copy it to the call-site
| requiring the flag isn't the better alternative.
| danabramov wrote:
| Yes. If extracting and deduplicating is in your toolbox, so
| should be inlining and duplicating.
| konschubert wrote:
| you can also try to reason about the situation to figure out if
| the duplication is incidental or inherent.
| crazy5sheep wrote:
| I don't agree the issues OP later discovered has anything related
| to `refactoring` itself, but more a issue of premature
| optimization.
|
| my 2 cents, an interface is defined, it shall not be modified for
| no good reason. Even if you do, you can still have some way to
| make sure it can be compatible with the original system. and I
| don't believe you can't extract some common behaviors of those
| repeated code, and use them within the interface, refactoring
| doesn't mean you have to rewrite the whole project, it can be
| done by piece by piece. reducing a line of duplicated code can
| save you a lot of efforts on maintaining the project in its life-
| cycle. a lot of times, I have seen a code change was made to fix
| some bugs were forgotten in other place which duplicated the same
| original code.
| whoisjuan wrote:
| If there's something that I have learned about refactoring code
| that is repetitive into "cleaner" shorter code, is that the
| refactored version looks better but it's way harder to
| understand. When other people try to look at the "cleaner"
| version they have to spend more time trying to understand it and
| mentally untangle the abstraction.
|
| I like syntactically shortcode as long as it's clear. I also
| understand that sometimes shorter code has some small performance
| advantages that can add up in languages like JavaScript where the
| size of the files can become a loading speed bottleneck.
|
| But to be honest it really bothers me when someone tries to make
| perfectly fine and readable code into something different just to
| satisfy some weird intellectual urge to make things more
| abstract.
|
| Sometimes long code is not only easier to read and understand,
| but it also helps to create a better technical outline of
| decisions that otherwise will get lost in the reasoning of
| whoever is writing that code.
| mattacular wrote:
| "First you learn the value of abstraction, then you learn the
| cost of abstraction, then you're ready to engineer"
| temac wrote:
| > A healthy engineering team is constantly building trust.
| Rewriting your teammate's code without a discussion is a huge
| blow to your ability to effectively collaborate on a codebase
| together.
|
| I'm against the idea that people should be attached to "their"
| code (that is: the code they wrote). Now I _also_ understand that
| humans that humans, but the priority should be to make them
| evolve toward more detachment from their work and acceptance that
| what they design "can" (actually will) be imperfect, rather than
| avoiding upsetting them even when it would not be justified to be
| upset. Plus one essential purpose of source control is to be able
| to make changes, and revert them if they were "wrong"; or for any
| other purpose. Maybe propose a patch instead of committing
| directly, but that's really a cultural matter about how projects
| are organized.
|
| I don't want to ask of permission for an improvement. If there is
| a need to have formal authorization of maintainers responsible
| for parts of the code, they setup just that. Otherwise, I'm
| certainly going to improve "your" code, _in some rare cases_
| without even telling you (that 's not a goal in itself to do it
| behind your back, obviously, and I also value collaboration --
| but that should not be a problem). The question that remains is:
| is it really an improvement. If you are not sure, then maybe
| don't commit. If you are, do it (following your local rules), and
| if it ends up being a mistake, yes, it will be reverted, and so
| what?
|
| You should not take issue of your work being reverted (for good
| reasons), like other people should not take issue of "their" code
| being modified. Better ask for forgiveness than permission.
|
| > For example, we later needed many special cases and behaviors
| for different handles on different shapes. My abstraction would
| have to become several times more convoluted to afford that,
| whereas with the original "messy" version such changes stayed
| easy as cake.
|
| Then re-expand the code later.
| tcbawo wrote:
| The quality of a team is the quality of the communication. If
| the architecture and design isn't well communicated, you get
| people going off in the wrong direction, or polluting a clean
| design. Also, people are most attached to their code based on
| their time investment. If you trash someone's code/changes, you
| trash their time, which could have been more efficiently
| decided with a conversation ahead of time. If you find yourself
| surprised by design changes, find design flaws in a code
| review, or have different definitions of "improvement", the
| communication in your company may be lacking.
| yxhuvud wrote:
| You want trust, but you also want code review. The example
| lacked both.
| the_unproven wrote:
| I also think it's fine to change the code someone wrote. Just
| because someone wrote it, doesn't mean it's the right way to do
| it. I often find myself rewriting the code, it's the natural
| process of code evolution. It just feels that it should be more
| readable, efficient etc.
|
| Although, if the change is essential or it requires more pair
| of eyes, I'll just make a PR(MR) and let the people review it.
| mannykannot wrote:
| There is a delicate balance: a sense of ownership can encourage
| pride and responsibility, but it can also impede cooperation.
|
| > Better ask for forgiveness than permission.
|
| Not in this case; you are likely to piss people off, especially
| if there is a consequence you have overlooked.
| skybrian wrote:
| In an environment where all code must be reviewed, when you
| make a change like this you send it to the original owner for
| review. Or at least someone on the same team, if they're not
| available.
|
| "Don't be too attached to your code" means trying to review
| improvements to the code you just wrote objectively. Can you
| accept changes with good grace?
|
| But you can't get to the point where you have a smooth-working
| team if you have philosophical differences about what
| improvement looks like! If one person's improvement is another
| person's regression then those difference need to be worked out
| and some kind of synthesis agreed to. This can take a lot of
| time, but it's necessary work to get to that ideal.
|
| On the other hand, a lot of work gets done by teams that are
| _not_ smooth-running teams that are in philosophical agreement.
| It may even be a good thing to have a diversity of opinion and
| avoid group-think? You have to be able to accept some messiness
| though.
| danabramov wrote:
| I think there's a balance. There's a difference between
| gradually improving something, or improving it at some point
| later -- and literally rewriting 100% of code someone has just
| landed over the night.
| slavik81 wrote:
| I agree with you. To me, the action felt wrong when he
| described it. I wasn't sure why at first, but I think perhaps
| because the work was not in service to any actual task.
|
| If he was in charge of writing the next piece of
| functionality for that code, it would be poor teamwork to
| completely refactor it without discussing the reasons for the
| design, but at least it would have been within his area of
| responsibility.
|
| This is also the reason why I like having code review. The
| review would have been a good place to raise his concerns.
| They could have had their eventual conversion earlier, before
| feelings were hurt or wasted efforts were made. It would even
| help in the opposite case, where the author's code was
| legitimately poor and the reviewer had good suggestions.
|
| It's not a panacea, but I find code review helpful. In my
| experience, review was the time when a lot of knowledge-
| sharing occurred. Some might feel that only senior developers
| should review, but as a junior I learned a lot about why the
| author made the choices they did. I learned how people
| expected the code to change, about language features or
| pitfalls, etc., and it helped me grow as a developer.
| rubatuga wrote:
| I mean, it's nice to talk about ideals, but it's also highly
| unrealistic that people are able to separate the code they
| write and what is says about them. Especially if you are part
| of a newly formed team, you must make sure that there is trust,
| respect, and clear communication, because this is the stage
| where most misconceptions occur. Upon building a successful
| foundation, you can test new team dynamics such as changes in
| communication that tradeoff that initial safety for more
| efficiency.
| pier25 wrote:
| Principles are not rules. Also, maintainable and readable code
| are of uttermost importance.
| rlonn wrote:
| Have to push this fantastic article again - if you read it and
| follow what it says you won't make these mistakes:
| https://programmingisterrible.com/post/139222674273/write-co...
| boyadjian wrote:
| I totally agree on this post. Being too zealous about code
| duplication is counter-productive. Don't be more royalist than
| the king.
| zallarak wrote:
| As PG said, take on as much technical debt as possible: it's
| literally leverage.
|
| Now sometimes "clean" code is the worst of both worlds. It wastes
| time and isn't really clean.
| aazaa wrote:
| > ... My code traded the ability to change requirements for
| reduced duplication, and it was not a good trade. For example, we
| later needed many special cases and behaviors for different
| handles on different shapes. My abstraction would have to become
| several times more convoluted to afford that, whereas with the
| original "messy" version such changes stayed easy as cake.
|
| The article's starting example and revision clearly illustrate
| how the original code was changed. Duplication was removed - just
| like so many advocate for.
|
| Unfortunately, the article doesn't state the changed requirement
| that broke the new design, leaving that to the reader's
| imagination. And I can imagine cases in which the revised code
| would be superior to the original.
|
| It sounds like the problem arose from the need for specialized
| handles on different shapes. If so, I don't see how using various
| createXHandle functions (where "X" is an identifier associated
| with a specialized handle) would necessarily cause convolution.
| It could become quite convoluted if not done with care. For
| example, keep the createHandle method, but pass customization
| parameters to it. But that's really the naive way to go.
|
| What we'd be faced with is nothing more than the kind of
| specialization requirement we usually see with Object Oriented
| programming. We can use composition over inheritance to great
| effect there. We can extend the author's refactored design and
| end up with something clean and maintainable.
| cmdshiftf4 wrote:
| So the two cases against writing the most legible, succinct code
| _given the specifications at the time of writing it_ are:
|
| >Firstly, I didn't talk to the person who wrote it. I rewrote the
| code and checked it in without their input. Even if it was an
| improvement (which I don't believe anymore), this is a terrible
| way to go about it. A healthy engineering team is constantly
| building trust. Rewriting your teammate's code without a
| discussion is a huge blow to your ability to effectively
| collaborate on a codebase together.
|
| There's no question about this. Nobody likes the self-proclaimed
| savant who works in isolation and makes sweeping changes to the
| codebase or other people's work without collaborating and gaining
| some consensus. If it's a change worth making it should be a
| simple case to present to your (hopefully) equally intelligent
| team.
|
| There is a difference, it has to be highlighted, between
| refactoring someone's code in order to extend it yourself and
| simply re-writing someone's implementation because it doesn't
| suit your requirements. The former is part of the job, the latter
| should at the very least be an opportunity to mentor the person's
| whose code you want to re-write in _why_ it was suboptimal and
| guide them on the changes you 'd like to make, or even give them
| the chance to make it themselves. This is kind of what code
| reviews are supposed to do.
|
| That does not negate the need to structure and optimize code to
| remove duplication whatsoever. It's not an argument against clean
| code standards and it's weak that it amounts to 50% of his case
| here.
|
| >Secondly, nothing is free. My code traded the ability to change
| requirements for reduced duplication, and it was not a good
| trade. For example, we later needed many special cases and
| behaviors for different handles on different shapes. My
| abstraction would have to become several times more convoluted to
| afford that, whereas with the original "messy" version such
| changes stayed easy as cake.
|
| Time changes, requirements change. It's part and parcel of our
| jobs in software development. Writing code that at one point is
| optimal and most legible for the cases present should also be
| done to try to make it refactorable and extendable.
|
| It is much easier to refactor and extend code that isn't riddled
| with duplication and mangled with hardcoded business logic.
| Abstract your code and write your implementations well, name
| things in a way that people can read it and write tests that
| describe what's expected from it.
|
| Refactoring well isn't easy work. Refactoring a sprawling legacy
| codebase with a lot of duplication and legibility problems is
| _significantly_ worse.
|
| I'm not saying we need to be dogmatic here. If you're given the
| opportunity to develop new code you should be aiming to do the
| best job of it given what you know _now_ , in a way that will be
| comprehensible to you, or whoever needs to touch that code
| _next_.
|
| We all know that there are problems with premature optimization
| caused by "best practices" evangelists who'd happily drive up
| time-to-market and operating costs/complexity exponentially in
| the name of having the codebase and applications / services
| architecture in line with whatever he or she has read lately from
| "thought leaders" in our industry, but writing the code for a
| given application in line with the above isn't one of them.
| scarface74 wrote:
| _It is much easier to refactor and extend code that isn 't
| riddled with duplication and mangled with hardcoded business
| logic. Abstract your code and write your implementations well,
| name things in a way that people can read it and write tests
| that describe what's expected from it._
|
| Especially in a statically compiled language - "extract
| method", "extract class", "pull members up", etc. is an
| automated, guaranteed safe refactor (ignoring reflection) .
| mntmoss wrote:
| The code is not large enough to need maintenance at a fine-
| grained level.
|
| There is a secondary rule to the DRY "rule of three": If I can
| blow it away and rewrite it so easily, there is nothing to
| reuse or refactor in it. The feature is done, and we are into
| code golf and speculation, neither of which are productive uses
| of time. In my experience the success rate of speculative
| refactors like the one author made has perhaps a 50/50 chance,
| so no better than the initial strategy. It's the requirements
| themselves and the application of techniques to avoid various
| classes of errors that give code direction and structure - not
| the aesthetics at a moment in time(which is what author took
| issue with).
|
| If you spot multiple approaches on the first try, you can add a
| comment with a date outlining alternatives so that the
| conversation may be resumed later when the new requirements
| come in. But at all times you're always at the mercy of
| "discipline", and there's no preemptive measure that avoids
| that.
| Rapzid wrote:
| The author also didn't sound like a particularly senior
| engineer at the time for many reasons. So the original author
| and the "boss" may have been taking into consideration
| timelines and future work/requirements coming down the pipe.
|
| A very valid reason could have been as simple as "We are re-
| visiting this in a couple sprints after feedback and will
| have a better idea of how it needs to change. The extra day
| spent on this wasn't worth pushing getting it into peoples
| hands, and we don't know if it would be a waste." The author
| would have known this if he started a conversation about it.
___________________________________________________________________
(page generated 2020-01-11 23:00 UTC)