Comment by aalleavitch | Hacker News Reader

Comment by aalleavitch | Hacker News Reader

Parent Full thread

This is absolutely the situation I am in right now, and I desperately need all the guidance I can get. We’re talking thousands of lines of React components that rely regularly on manipulating global js variables, mixed with MVC 5 written in Razor delivering variables in script tags, mixed with outdated jQuery, that’s all just concat’d together with gulp and that hasn’t seen a refactor in years of changing hands with no style guide. The company started in 2004 and I swear some of this code is still floating around from the day it started. We have three sets of scss breakpoints, three or four layers of “common.scss” files that act like a piece of furniture that has been painted over dozens of time (with shit like * {padding:5px} floating around), and an unknown amount of dead code. There’s lines written back-to-back with vastly different styles (with a random distribution of semi-colons and arrow functions and etc), and major global variables named with misspellings. And while I clean this all up, I am expected to continue pushing out features and addressing serious bugs with stop-gap measures.

Please advise.

Welcome to code development in the real world 101.

You either go insane, give up and take up beekeeping, start your own company or finally decide to not give a shit anymore and start hacking the same way as everybody else because in the end your are not sending astronauts to Mars, and it works "most of the time" is good enough for the clients and the price and time they are willing to pay.

> take up beekeeping

I thought the idea was to have less bugs to worry about...

Yeah. On the whole, it is less. ;)

Real world 303.

Work for a company and project sending astronaut to Mars, then you can care and be surrounded by coworkers who care too.

You need to tell your manager that it's simply impossible to both clean all that up and also make progress on the project. You need to stop the erosion in order to fix it; fixing will require fundamental changes that will absolutely block progress. If aren't able to freeze the project, your cleanup progress will be constantly thwarted by new ugly hacks.

There's just no way around it. If your managers won't freeze the project until you make meaningful progress, you're going to be stuck working with this ugly code for a long time. Try to convince them that in the long-term a cleanup will pay dividends. Without their support though, you won't get anywhere.

If you're able to freeze the code for a bit, it sounds like it might just be quicker to rewrite some parts like the CSS. Don't take on the whole project at once - find pieces that can be rewritten individually, even if it's a large part like all the styling.

Unfortunately there are business reasons why freezing the code is impossible. We’re ecommerce and working on a timeline dictated by consumer behavior. So if we need a promo tomorrow it’s going live or else we’re not making budget. I kind of assume this is how we got into this state to begin with.

And unfortunately nothing will change because what you want to do is better in every single way, but their way of doing thing is better in the only one that actually counts: It's faster and cheaper.

My assumption is that I will never clean up this entire codebase. Rather, I’m looking for the best way to triage the situation: what are the things I can/should try to fix first?

Write down everything you know about the codebase. Whenever you discover a surprising dependency or behavior make a note of it. This is something you can do in passing as you push features.

At some point you'll start recognizing structures in the code some of which will be small enough to surgically improve.

Make your problem their problem.

Make a list and organize all the things you'd want to change. Untangling balls of mud often take on the order of months to start seeing any kind of progress. You have to make sure you keep things organized over a long time otherwise you can't tell whether or not you are making progress.

If a module breaks often when seemingly unrelated things change or breaks every time someone touches it it's easy to say the module is badly written. It's often hard to say why it's bad and harder to say how to fix it. However at least you will have data and be able to defend your position that messy code in a particular module actually slowed down the addition of a new business features.

If you are using a bug tracking tool (Jira, Bugzilla etc) make a bunch of private ones for yourself. If management doesn't like all your private tasks showing up on reports then try to get a section for yourself or see if you can deploy your own "private" bug tracking instance.

Any time you encounter a problem make a bug for it even if you have no solution. If upgrading a library version a minor version causes a break make a bug for it etc. You'll start to see patterns and then you can make small but really important surgery on the code. You can have a plan for how to fix a particular module when the time arises.

For example we had a core module with some 190 downstream internal clients across some 5 different dev teams. A recent Oracle version upgrade caused problems because we would have to to coordinate everyone to update at the same time. I already had a long time standing issue that the module is poorly organized and already had a list of all the things that were wrong with it. When this issue came up I immediately stepped in and how to completely remove Oracle from the core module so that the downstream clients can each upgrade on their own time. Because I had thought about this for time I was ready to defend the proposal by showing it would take less time to do this. I was able to do a refactoring I've been wanting to do. Over time people will start to listen to you more if you are actually delivering shit on time. Otherwise, even if your complaints are correct, you will be labelled as just a whiner.

I disagree with most of this comment, but I think it's an interesting and valuable discussion.

Here's where we agree: You need to stop the erosion in order to fix it. Every developer on the project needs a shared vision of what "better" means. For example, the parent comment mentions React components that manipulate global variables. First of all, everyone agrees to stop doing that, and agrees on the alternative (use props, a redux store, whatever).

But you don't stop the world and rewrite to the new standard. Instead, you apply a strong "boy scout rule" to every code change. If a developer modifies a small component, they also need to remove the global references. If the existing component is large, the developer must extract a smaller one, and the new extracted component must meet the new standard.

This will slow your velocity - at first, by a lot. But you never stop delivering entirely. You never waste time rewriting things that you would never touch again anyway. You also learn along the way - because while "don't use globals" is pretty universal, most architectural decisions are not. You will find that the "clean new standard" changes to match your application. At the same time, everyone on the team learns about the new standard, and buys in to the new way of doing things. There isn't a "new system" with the cool kids, while the B-team strings along the old one and waits to be laid off.

That's what I've always done. Fix it when you see it. Eventually, you have a clean codebase.

I've mixed feelings on this still. It leads to big code reviews, with most of the changes being less important. So I rather like the separation, spend some time just for cleanup. Maybe take notes on wins as you see them but you don't need to do them yet (or put them in the same review as another bug fix / story item).

If you're using git and can separate things out cleanly then I think fixing it as you go would work better. I yearn to use git every day again...

Well, I'm not talking about a major refactor (squash them when you see them). This should be an enum, this is named poorly, this variable is scoped wrong, inline sql when should be a sproc, that kinda stuff. A few lines a fix, the low hanging fruit.

Major refactors certainly need to be accounted for in a sprint(s), but keep features a priority when needed in sprints.

How will you make sure that you are not introducing regressions in such code base? You cannot add unit tests because it is probably untestable mess. So instead of some uncertain dividends, which may or may not materialize, you introduce real risk of breaking already existing stuff.

For "light reading" on this subject, try _Working Effectively With Legacy Code_ by Michael Feathers. We used it to very good effect at my last company.

Michael Feathers was on Software Engineering Radio last year: http://www.se-radio.net/2017/06/se-radio-episode-295-michael...

Personally I thrive in this kind of environment, it's a kind of comfortable chaos. MVC5 is positively modern compared to many of the products I've worked with which have included (within the last 5 years) VB6 winforms but mostly aspx webforms.

Understand that previous developers might not know what a DOM is have likely written ad-hoc jquery snippets, included 3 different versions of jquery-ui until they happened across the right combinations of versions to get their plugins to play and will have a mentality of "it was working".

Given that, here's some advise on how to stay sane:

1. Read and "Digest Working Effectively with Legacy Code" but accept you'll likely not be empowered to actually implement the changes.

2. Understand you'll have a grace period of ~6 months where you'll be far more empowered to make changes and get suggestions pushed through, after which you'll likely find yourself worn down by the system and more accepting of the status-quo. Leverage this time if you can.

3. Try to identify what caused the product to get to the state it is so it can be avoided for future products. If it's something you're not empowered to change such as "bad hiring policy" try to work out how you can spot that in future interviews.

4. Be pro-active in looking for bugs, particularly security flaws. Finding an IDOR which lets you get cross-account data is usually fairly straightforward in these kinds of environments. Doing so gets you noticed so you're not just another cog in the system and can provide real value. Be careful not to put others' out too much while doing this. It sounds like you only work on the front-end but even what appears to be front-end can have security implications such as template-injection.

5. Draw a line in the sand. Be entire accepting in legacy code but if someone is working on a new part of the product or is fixing a bug in legacy code be absolutely ruthless in what you'll let passed in code review. This may not do you any favours socially but a reputation for harsh code reviews isn't all bad. For one thing you'll have to do fewer code reviews.

6. Don't bite off more than you can chew. Treat it like a giant knot and don't be tempted to pull at threads unless absolutely necessary. Instead wait until given strong reasons for refactoring.

Edit: formatting, because HN doesn't do markdown.

> if someone is working on a new part of the product or is fixing a bug in legacy code be absolutely ruthless in what you'll let passed in code review. This may not do you any favours socially but a reputation for harsh code reviews isn't all bad.

In my experience this is the most important thing. I particularly focus on the "what the heck does this code even do" aspect - the person who is modifying the code needs to be able to explain exactly what the code does and why it is correct (don't accept "it compiles and the one unit test we have passes"). Forcing the team to understand the code will make it very difficult for them to continue adding bad code to the pile, as they won't be able to explain what they bad code does, or will realize the code is bad in the course of trying to explain it.

It is very, very important in this situation, however, to not be outright mean. Do not let anything through that you would not be comfortable maintaining, but don't be a dick about it. All that will end up accomplishing is that you'll be blamed for things not going through, and then you'll lose your credibility as steward of the code. People will go over your head to the boss, who will just make you pass it.

It should be noted that more often than not there are strong reasons besides ignorance and incompetence for a project to arrive at a certain state.

https://en.m.wikipedia.org/wiki/Wikipedia:Chesterton%27s_fen...

It shoud be stressed that a developer should not assume all his predecessors were wrong just because he doesn't understand why some stuff ended up being the way it is. The world is packed with developers who proud themselves of fixing things that were never broken, and in the process break stuff.

Unfortunately 5. doesn't work if your company doesn't do any code reviews.

In regards to #5, worth pointing out that some coworkers are probably just as disappointed with the current state of affairs as you. They would probably really appreciate somebody pushing them to hone their craft.

Also worth considering that some people never learned how to handle constructive criticism and will take any suggestions as personal attacks. Stay as far away from these people as possible, you aren't going to change them but their pettiness can make your life miserable, particularly if they're closer to management than you are.

You should trust your engineering judgement, and your tech lead should listen to you when you raise these issues, and your product team should give your team engineering time to start cleaning things up. If any of this doesn't happen, get a different job.

As engineers we have a responsibility to raise concerns about bad practices and projects that have been neglected this way. Doctors raise concerns about hospitals, lawyers raise concerns about firms and enterprises, structural engineers raise concerns about buildings and infrastructure. Software engineers should do the same, resigning in protest if need be.

First agree on what it should actually be and document that. Make sure nobody commits anything that makes the codebase worse. Don’t refactor but clean first, and only while delivering value.

Rule 1: No new bullshit hacks make it in.

Rule 2: One bullshit hack must be removed per sprint.

Over time, you'll have better software.

Reply on news.ycombinator.com