Always leave the code better than you found it

https://letterstoanewdeveloper.com/2020/11/23/always-leave-the-code-better-than-you-found-it/

by inductive_magic • 6 years ago

49 70 6 years ago

letterstoanewdeveloper.com

meowster• 6 years ago

Off topic, but the title reminds me of Scouts: Always leave a place cleaner than you found it.

I think it's a good philosophy, whether it's about code or trash. It makes everyone's life better overall.

casion• 6 years ago

Until you make a bunch of choices about "better" that are based on presumptions that are incorrect. Now you're just making things worse and filling yourself with a sense of accomplishment that puts you out of touch with reality.

When I was in scouts, our scout leader was adamant that we should leave the place "The way it would have been if left alone" and not try to push our own ideas of "better" on things. Which I think is a much better line of thinking, even if still flawed.

joncampbelldev9• 6 years ago

Whilst improvements shouldn't be done ad-hoc without consideration, I think the concept of "left alone" applies more easily to the concept of natural spaces. In code it is really stretching the metaphor to do much more than very basic tasks like renaming or code formatting (e.g. picking up trash). Adding a test doesn't feel like "left alone".

One of the examples in the article is upgrading a dependency. This could easily fall into the category you mention if the upgrade itself is not properly tested. A considered approach would be:

"I want to make a small improvement, oh I see this dependency is out of date and has some minor vulnerabilities or perf issues, or is missing a feature that could come in handy for XYZ. Hmm, there are no tests confirming our assumptions about the current library version, nor are then any tests are our code / systems that use it. I guess my improvement should be to add those tests, then next time someone has the same idea it'll be an easier upgrade"

I am unsure but I also interpret from your first sentence that you may have seen people use this rule as a defence for gold plating (the opposite of KISS or YAGNI). Similarly this should fall under the heading of changes made without proper discussion, thought or consideration.

Otherwise I think we're really stuck, if team members cannot be trusted to properly interpret the boy scout rule without messing up a codebase then how on earth can they be trusted to build the foundation of their company / team / system?

paulintrognon• 6 years ago

The funny thing is, is leaving the land "the way it would have been if left alone" better? In many cases, it is debatable.

In New Zealand, there are many invasive species of animals and trees. Should they interfere and try to remove the invasive species? Or you should they "leave it alone" and let the pine trees take over all native plant species? (the same is happening in the french Alpes where pine trees are invading a lot of land).

skrebbel• 6 years ago

That's why it's called "the boyscout rule" and the verb "to boyscout".

Eg in a PR description "I also boyscouted the entire login service a bit while I was in there".

bank90210• 6 years ago

Ugh...had a company consult for us who had this as their moto. A guy decided to refactor all the code the way he felt it should be. It had no concept of underlying logic and reason why previous decisions were made. It broke many systems and the guy was fired, but we were left with his crappy code.

joncampbelldev9• 6 years ago

This sounds very different to the process described in post. Are you raising this anecdote as an argument against the boy scout rule?

I don't think you would find many people who would consider his actions a reasonable interpretation of it. I think this person would cause damage regardless of their motto or underlying intent.

Taking a leaf from the guidelines for person-to-person,discussion on this forum about arguing against the strongest version of your opponents argument: I think there is great value to lots of small improvements over a long period of time (no silver bullet etc). Certainly it should be tempered with the knowledge that the change is indeed an improvement. That can usually only be decided by the person who originally wrote it OR a consesus of the team involved.

dahfizz• 6 years ago

The problem is that there is no objective measure of "better". The consultant did think he was leaving the code better than he found it.

I've worked on codebases where the whole team agrees a certain part of the codebase needs to be refactored, but we all also agree it is way too dangerous. Which is better? Working but hard to maintain spaghetti code, or clean, new, and broken code?

sys_647384• 6 years ago

Sounds like the ‘what problem are they trying to solve was I’ll-defined’. Consulting companies will always find issues as that’s their reason for having the account - to generate revenue.

mooreds2• 6 years ago

Author here. Thanks for your feedback. Many comments are from people with your take, who've apparently gotten burned by someone "improving" the code by reworking code they didn't understand.

(See also the discussion when I posted this on HN: https://news.ycombinator.com/item?id=25198597 ).

That's not what I intended, which is why I have this in there:

Often you’ll jump into code to fix a bug, investigate an issue or answer a question.
When you do so, improve it. This doesn’t mean you rewrite it, or upgrade all the libraries it depends on, or rename all the variables.
You don’t need to transform it.
But you should make it better. Just clean it up a bit. Doing so makes everyone’s lives just a bit better, helps the codebase in a sustainable way, and assists the business by making its supporting infrastructure more flexible.

Perhaps I need to make it super explicit that you should understand whatever changes you are making, discuss them with team members, have them code reviewed, and start with small steps (like documentation). I thought that was implied with the above, but maybe I wasn't clear enough.

we were left with his crappy code

Seems like a horrible situation. I have so many questions!

If it was a true refactor (and not new functionality) why did you have to use the crappy code? Or did he do a rewrite in the process of adding new functionality? Who was reviewing the code? How did it get merged to the mainline?

joncampbelldev9• 6 years ago

Your article was very clear. Thank you for writing it.

It is a common refrain against simple mottos or ideologies in programming for people to pick the worst memory they have of a coder or experience as an example of why it cannot work.

As if the problem with a bad team member is that they heard the wrong motto, if only they hadn't started incorrectly following the boy scout rule their contributions would be exemplary.

sethammons• 6 years ago

Fwiw, from the article:

You don’t need to transform it. But you should make it better. Just clean it up a bit.
A warning about refactoring. Don’t refactor what you don’t understand. Don’t drive by refactor. Discuss your plan with someone more familiar with the code.

convolvatron• 6 years ago

the point of refactoring isn't reflected in the code

if your organization has lost its grip on the code base, no one understands it, and every time you try to touch it something odd breaks and you have to spend days tracking it down - you have a problem.

if you can talk to someone about what the code does, and there is sufficient testing in place so make it safe to change - you don't have a problem.

refactoring is the process of (re)taking ownership of the codebase. the result is in your head. you don't even need to get the result committed for it to have been useful.

bluedino2• 6 years ago

If the function is ass-backwards and fucked up, add your feature or bugfix in the same fashion.

It's probably that way for a reason.

Every time I tried to clean up code, I ran into the following:

  Some arcane bug was exposed
  The previous developers couldn't understand it 
  Something else broke

So unless you have a way to 100% test your change, keep the status quo.

ironmagma• 6 years ago

it’s probably that way for a reason

I find this assumption to be harmful. There’s really no probably about it; it’s just the way it is, might have been intentional, might have been completely unwitting. The solution should be to refactor if you have to but in a way that doesn’t change the underlying functionality.

If you think about it, there is an extreme case as a contractor where the code you need to change is so convoluted and impossible to understand (imagine a neural net without the training set) that you just can’t change it without refactoring. So I would argue refactoring is necessary sometimes.

alisonkisk2• 6 years ago

You hired someone to change your code and didn't code-review it, have tests, or ask for tests to be written? There's a bigger problem there than a refactor-happy consultant.

notacoward4• 6 years ago

Sounds like you're talking about a rewrite, not a refactor. The whole idea of refactoring is to make code less repetitive or modular without changing functionality. Common patterns include replacing copy/paste code with common functions, replacing complex inline conditions and if/else cascades with helper functions returning bools/enums, etc. A true refactor is provably neutral wrt functionality, often so by simple inspection. If you're changing functionality that's a rewrite, and it's not making the code better because the original code no longer exists.

burade2• 6 years ago

How do you know you're not introducing a bug by replacing copy/paste code with common functions?

Most of the production code I've touched I would never dare to do something like this until I had a very good understanding of the codebase as a whole.

TruffleLabs• 6 years ago

Refactoring is a form of rewriting code.

hinkley6• 6 years ago

Well-meaning people can make a big mess and people let them because their heart is in the right place, even if their head is up their own ass.

I’ve resigned myself to the idea that there’s two diametrically opposed way to run a software project.

One is by rote memorization. The people on the team have been there a long time. Seniority is hard to achieve because everyone makes dumb mistakes for a few years, instead of six months, due to the code being rife with tribal knowledge. This group gets very, very cranky when you refactor, because you are moving things out of place. Things they spend a great deal of effort memorizing. You’re threatening their seniority, their job security, and their control.

Another way is to design for people getting in and out quickly. The knowledge is curated, the code tells a story you can follow. Important assumptions are encoded plainly, perhaps documented there or in a wiki. This team can size up or down more quickly without bloodshed. Which is great if there are other projects that need attention, not so good if the company is a one trick pony with money flow problems. Counterintuitively, your best people may choose to stay a little longer here because they don’t feel trapped.

Early in my career I had a lot of luck changing engineering culture for the better, and I mistakenly assumed these techniques would work anywhere. They work lots of places, but they don’t work on a team that is collectively smarter than you are. Like doctors, clever but misguided developers can make the worst patients. There was one team in particular in a vertical I really wanted to be in, where I should have quit four months in, when it was obvious we would never see eye to eye. How can you hate your own code but refuse to change how you write it? Sometimes the smartest developers are the dumbest people.

Good severance package when the layoffs happened, but I suspect to this day the manager thinks I was part of their decline, rather than the harbinger.

In a way we are both right. My energies would have been better spent elsewhere. I’ve thought a lot about how to detect this sort of dynamic at new prospects but the best I’ve come up with is maybe don’t take a job where you’re the first new hire in n years and everyone else has been there forever. It might be okay, but it might be hell on wheels.

sys_647384• 6 years ago

Who reviews code changes by the developer?

joncampbelldev9• 6 years ago

Other developers, unless it's a solo dev at a small company. Then roll the dice and hope you hired someone with good design sensibilities.

sys_647384• 6 years ago

So you’d hope this and the prior design review would stop the consultant from wasted effort. Probably not.

umvi• 6 years ago

Great article. A lot of times even just the first one (Documentation) is immensely helpful.

For example, one time I went through a comment-less source file full of mathematical formulas and replaced magic numbers with meaningful constants and documented which RF chipset datasheet each formula/constant was coming from (including page number). Other developers thanked me later when they were looking for the same thing.

forest_dweller• 6 years ago

It really depends on the organisation. This sounds sensible, pro-active and productive until to work on legacy systems that have been in place probably or organisations that don't value any of these activities (you typically don't want to stay there but sometimes you just gotta put up with a role until you get a chance to move on).

Generally in this scenario your job is to make a change that fulfils the requirements, verify that it works and test it manually, pass it onto QA and move on. Businesses often don't care your improvements unless it is a new feature and it will make you look bad because it will take you longer to complete a task than they deem necessary.

As for refactoring in this situation. Don't. Even making seemly simple changes can it makes it difficult for others to code review it, or could incur breakage (yes there are some systems that are this fragile).

tuwtuwtuwtuw7• 6 years ago

The issue I run into with this comes part of the code review.

So some developer is going to fix some code, and the code is ugly. So the developer fixes the bug and makes the code prettier and push it as a single commit. I am then left with figuring out which changes are related to the bug fix and what is related to the cleanup.

If I tell the developer to first fix the bug and then cleanup in a separate commit, it may be inefficient since the fix after cleanup may not be the same as fix before cleanup. It may also lead to the cleanup being skipped because his work is technically done after the fix.

If I tell him to cleanup first and fix later, it's also seem to lead to issues since the developer has typically already figured out how to fix it since he came into the code with the mindset of fixing the bug.

Of course I like the code being cleaned up, and I like the idea that you improve the code as you work with it. But I'm not sure what the best method is. Maybe the issue is with me, that I want the cleanup and fix in separate commits for easier reviewing.

Personally if I spot ugly code when fixing a bug I add unit tests, fix the bug and then add another ticket to improve the code. I then plan that code cleanup for this or the next sprint. But it feels tedious and is probably inefficient as well

goalieca• 6 years ago

I am then left with figuring out which changes are related to the bug fix and what is related to the cleanup.

Hmm. That has never been an issue for me in practice unless they go way too far with the refactoring. Usually it’s me who suggests cleanup for defensive coding reasons because the bug is often a coding issue.

hinkley6• 6 years ago

People frustrate me by taking the short view of these things.

Bigger refactorings and architectural changes usually require tracing the tendrils of an assumption out through the code. It’s not a matter of “can they figure it out,” it’s a matter of doing that in ten places and still somehow hold the other three bits of critical data in short term memory. If this path isn’t fruitful then there are five other theories to try so it’s not particularly amenable to writing it down. It’s flow state depth first search, and the crappy non-idiomatic code is ruining the flow.

People always try to deflect based on some intelligence argument. I’m not trying to understand your code, I’m trying to understand something you missed. This isn’t about intelligence, it’s about wisdom.

karmakaze• 6 years ago

Solution is to merge one git branch with three commits: new feature+new tests, (optional) new tests for refactor, refactor. I do this using git reset --soft when I'm done, even if it means having to split up changes made to the same file across commits. Save the original work and diff new complete changes with original set.

hinkley6• 6 years ago

Yes, you absolutely ask them to do the refactor first. Because that’s how grownups code. If they want to mature as programmers, they need to think about longevity of the code. You can’t just keep spackling over problems again and again and expect any sort of integrity to remain.

Make the change easy, then make the easy change. If they don’t they should expect comments. Code reviews are at least as much about setting standards and expectations as they are about catching new bugs.

tuwtuwtuwtuw7• 6 years ago

I think it would be wrong to set such a hard rule. You may have a bug which you could fix in say 2 hours but refactoring the code as a whole take 5 days. Is it always the "grownup" way to tell the customer that they can't use the system the next 3 days? I think it sometimes makes more sense to fix the code as is and then schedule a refactoring.

amw-zero6• 6 years ago

There’s nothing wrong with optimism. But this line of thinking is a bit naive unfortunately.

I’d say pretty much no one agrees on what “better” looks like. Better to someone often means applying a pattern that they just discovered in a blog post that week. Software design is still entirely subjective.

For example, “better” once meant refactoring to be more object-oriented. That’s backfired a bit recently, so now “better” might mean having a more functional design. There’s no evidence that either of these actually have any impact on the end result of quality as perceived by users.

Anyway, it’s good to be positive. There’s probably subconscious good that comes from that.

tuwtuwtuwtuw7• 6 years ago

I think in a huge number of cases it is possible to objectively say that a change made the code better. I see those very often when consulting at companies.

Just to give you an example (which I ran across last week), if you have some code handling a cart in an e-commerce site and you need to keep track of the number of items in this cart would you prefer the variable name "totalAmount" or "quantity". There's also a variable named "totalAmountIncludingTax" which holds the monetary amount to pay for the items in the cart.

mattgreenrocks4• 6 years ago

The accumulation of many small wins in things like naming is often much more helpful than full-on overhauls. But you have to be willing to do the small, seemingly inconsequential things to build up to those.

mattgreenrocks4• 6 years ago

Better to someone often means applying a pattern that they just discovered in a blog post that week. Software design is still entirely subjective.

No. There are plenty of not-purely-subjective criteria to justify refactors:

* the refactor improves abstraction quality

* design improvements that result in edge cases being obviated

* coupling is diminished/cohesion is improved

The relativistic "there is no perfect software design" view is good inasmuch that you don't make idols out of any particular design approach. It is bad when it becomes a fatalistic, thought-terminating meme used to justify one's own inaction. Software design is difficult work. That doesn't mean it isn't possible because the Internet hasn't figured out an exact recipe for creating it. :)

amw-zero6• 6 years ago

the refactor improves abstraction quality

The quality of abstractions is entirely subjective.

coupling is diminished / cohesion is improved

This is also entirely subjective. The idea that eliminating coupling is inherently good is an unsound premise. There’s no proof that coupling is bad, and even if there were, there’s also the conversation to be had about which things should be coupled, which is surely a subjective conversation.

design improvements resulting in edge cases being obviated

I guess this one is slightly closer to being an objective good. Though even still, I think this is linear thinking at its finest. You may be getting rid of some edge cases but introducing others. Every design is a trade off, and evaluating tradeoffs is a subjective act.

wufufufu2• 6 years ago

I applaud this, but I also advise against this for your career. Unless the change you're making has immediate and significant business benefits, from a PM/EM perspective you're 1) wasting time 2) potentially introducing bugs

The correct move is to wait until the org is so bogged down with tech debt that meaningful progress cannot be made, then switch companies/teams.

tuwtuwtuwtuw7• 6 years ago

Just to be clear, this is a joke right?

wufufufu2• 6 years ago

I didn't intend it as a joke. Maybe at a developer-friendly company it could be viewed as a joke.

burade2• 6 years ago

It will only be joke once our profession unionizes. Until then, nope, not a joke. It's actually very good career advice.

mzakharo1• 6 years ago

People who think like this should be fired on the spot.

notacoward4• 6 years ago

That would be >50% of the developers at all FAANG companies, >80% at Facebook.

mooreds2• 6 years ago

Oof, harsh assessment of our industry.

sigsergv• 6 years ago

There are some very valid points there: improve documentation or test. But others are not: refactoring (this should be done explicitly, with proper testing and approvement from management), dependency upgrade (the same reasons).

basicallydan• 6 years ago

A very nice sentiment!

I think one of the most useful parts of this list of suggestions is to write or improve tests. You did allude to this, but I think it’s the most useful part (if test coverage starts out poor) because by doing so, especially before doing any other changes, you’re making it easier to do some of the other suggestions with confidence.

Good read!

Always leave the code better than you found it

Related Stories