4 ms·

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 under

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.

joncampbelldev 6y 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 6y 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?
  - joncampbelldev 6y ago
    So for the consultant: Yes he did, clearly this consultant was an absolute disaster. An excellent cautionary tale of arrogance combined with lack of ability and design thinking. Changes made without thought / design or discussion with the team are unlikely to be "improvements", unless they're being made by someone intimately familar with the codebase (which an outside consultant cannot be). For your second point, I don't really see the applicability of this specific concept of the boy scout rule to the situation of "business critical code that is in such a bad state the whole team agrees it should be rewritten". However, were I to try to apply the concept to your situation, the first question to ask is "why are people afraid to change the code?". There's usually 2 reasons for this. Firstly, the code and the problem are super complex and hard to understand, this just takes a lot of time, to develop the knowledge and intuition around the code. Secondly, the code lacks tests (unit / intergration / functional / manual spreadsheet of test cases) that assert the behaviour you expect. Therefore my (unasked for and probably already known to you) suggestions for "leaving it cleaner than you found it" would be to make a start on understanding the code better through a series of very very very small refactorings. Pick one tiny function that is a mess and clean it up. At the same time, ensure your changes are correct by asserting all expected behaviour of that function. If you are now saying "duh of course" well thats the point. I do not think a single reasonable person would suggest that a full rewrite of a complex business critical system that is either poorly understood, poorly tested or both is a charitable interpretation of what the article is talking about. At the end of the day it is super easy to shoot down any idea or concept in programming, because we have so much power to abuse and ruin anything day to day if we're not careful. It's the easiest thing in the world to take a machete to a codebase without thought. But the problem there is the person holding the machete, they can't be trusted with it. I cannot think of a single thing that wouldn't be misinterpreted or done badly by some of the terrible coders I've met during my career.
  - cjfd 6y ago
    I think the all or nothing view may be the problem 'maintain spaghetti code' vs 'clean/new code'. When one inherits a code base like that one should start small. Write an automated test that tests a single scenario, add a comment that explains something particularly difficult that a developer spent a lot of time on finding out. Rename one single variable somewhere. Then gradually as one gains more knowledge start doing bigger refactors and bring more of the code under tests. It sounds like what has happened here is that a big bang rewrite was undertaken. This is almost always ill-advised.
    - joncampbelldev 6y ago
      Thank you for explaining much more clearly and succinctly than me.
    - hinkley 6y ago
      I like to use the model that humans can only grasp so much, and that only increases slowly if at all, so on a mature project you have to try to keep the amount of stuff to remember constant. New code that adds new concepts needs to be counterbalanced by simplifying things elsewhere. As the inherent complexity increases, chip away at the accidental. I sweeten the pot with management by pointing out that if you do this, it’s easier to ramp up new people. If you can ramp up new people, you can absorb new customers without making a fool of yourself in the process. We should want to be able to take on big new customers, right?
sys_64738 6y 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.
mooreds 6y 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 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?
- joncampbelldev 6y 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 6y 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 6y 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.
bluedino 6y 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 6y 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.
  - bluedino 6y ago
    One example was a quantity field that could say “warehouse”, which meant there was quantity in the warehouse but some manual process needed to be done. Why were they using a quantity field for more than just numbers? Why was it only being used in some corner of the code in a totally different module?
alisonkisk 6y 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.
notacoward 6y 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.
- burade 6y 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.
  - notacoward 6y ago
    IMX it has often been pretty obvious by inspection. If two or more functions do "if {complex condition} then {complex action with only one variation}" any competent programmer should be able to see that a modification using a common function is equivalent. A slightly more complex case might require more review eyeballs to prove equivalence. Then there are tests. For a relatively small piece of the code you can test quite exhaustively. Most often those tests can live on as permanent unit tests. Occasionally they're so tied to the implementation that they're better thrown away after they've accomplished their purpose. Obviously any but the most trivial refactor requires some care. Is it worth it, when (in the context of OP) you're in there to do something else? Sometimes yes, sometimes no. One rule of thumb is whether you're just rearranging code or changing the fundamental way that it works. Refactors of the first type can generally be kept low-risk and are probably worthwhile. Rewrites of the second type involve more risk and are probably better left to be projects unto themselves.
- TruffleLabs 6y ago
  Refactoring is a form of rewriting code.
hinkley 6y 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.

joncampbelldev 6y 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 6y 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?
- joncampbelldev 6y ago
  So for the consultant: Yes he did, clearly this consultant was an absolute disaster. An excellent cautionary tale of arrogance combined with lack of ability and design thinking. Changes made without thought / design or discussion with the team are unlikely to be "improvements", unless they're being made by someone intimately familar with the codebase (which an outside consultant cannot be). For your second point, I don't really see the applicability of this specific concept of the boy scout rule to the situation of "business critical code that is in such a bad state the whole team agrees it should be rewritten". However, were I to try to apply the concept to your situation, the first question to ask is "why are people afraid to change the code?". There's usually 2 reasons for this. Firstly, the code and the problem are super complex and hard to understand, this just takes a lot of time, to develop the knowledge and intuition around the code. Secondly, the code lacks tests (unit / intergration / functional / manual spreadsheet of test cases) that assert the behaviour you expect. Therefore my (unasked for and probably already known to you) suggestions for "leaving it cleaner than you found it" would be to make a start on understanding the code better through a series of very very very small refactorings. Pick one tiny function that is a mess and clean it up. At the same time, ensure your changes are correct by asserting all expected behaviour of that function. If you are now saying "duh of course" well thats the point. I do not think a single reasonable person would suggest that a full rewrite of a complex business critical system that is either poorly understood, poorly tested or both is a charitable interpretation of what the article is talking about. At the end of the day it is super easy to shoot down any idea or concept in programming, because we have so much power to abuse and ruin anything day to day if we're not careful. It's the easiest thing in the world to take a machete to a codebase without thought. But the problem there is the person holding the machete, they can't be trusted with it. I cannot think of a single thing that wouldn't be misinterpreted or done badly by some of the terrible coders I've met during my career.
- cjfd 6y ago
  I think the all or nothing view may be the problem 'maintain spaghetti code' vs 'clean/new code'. When one inherits a code base like that one should start small. Write an automated test that tests a single scenario, add a comment that explains something particularly difficult that a developer spent a lot of time on finding out. Rename one single variable somewhere. Then gradually as one gains more knowledge start doing bigger refactors and bring more of the code under tests. It sounds like what has happened here is that a big bang rewrite was undertaken. This is almost always ill-advised.
  - joncampbelldev 6y ago
    Thank you for explaining much more clearly and succinctly than me.
  - hinkley 6y ago
    I like to use the model that humans can only grasp so much, and that only increases slowly if at all, so on a mature project you have to try to keep the amount of stuff to remember constant. New code that adds new concepts needs to be counterbalanced by simplifying things elsewhere. As the inherent complexity increases, chip away at the accidental. I sweeten the pot with management by pointing out that if you do this, it’s easier to ramp up new people. If you can ramp up new people, you can absorb new customers without making a fool of yourself in the process. We should want to be able to take on big new customers, right?

sys_64738 6y 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.

mooreds 6y 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 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?

joncampbelldev 6y 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 6y 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 6y 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.

bluedino 6y 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 6y 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.
- bluedino 6y ago
  One example was a quantity field that could say “warehouse”, which meant there was quantity in the warehouse but some manual process needed to be done. Why were they using a quantity field for more than just numbers? Why was it only being used in some corner of the code in a totally different module?

alisonkisk 6y 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.

notacoward 6y 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.

burade 6y 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.
- notacoward 6y ago
  IMX it has often been pretty obvious by inspection. If two or more functions do "if {complex condition} then {complex action with only one variation}" any competent programmer should be able to see that a modification using a common function is equivalent. A slightly more complex case might require more review eyeballs to prove equivalence. Then there are tests. For a relatively small piece of the code you can test quite exhaustively. Most often those tests can live on as permanent unit tests. Occasionally they're so tied to the implementation that they're better thrown away after they've accomplished their purpose. Obviously any but the most trivial refactor requires some care. Is it worth it, when (in the context of OP) you're in there to do something else? Sometimes yes, sometimes no. One rule of thumb is whether you're just rearranging code or changing the fundamental way that it works. Refactors of the first type can generally be kept low-risk and are probably worthwhile. Rewrites of the second type involve more risk and are probably better left to be projects unto themselves.
TruffleLabs 6y ago
Refactoring is a form of rewriting code.

hinkley 6y 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.