269

If I were in your shoes, I would probably try it this way: first, finish the current project - at least partially - as soon as possible, but in a working state. Probably you need to reduce your original goals, think about the minimum functionality you really need to see in "version 1.0". then, and only then think about a rewrite from scratch (lets call ...


224

It seems you are refactoring "just in case", without knowing exactly which parts of the codebase in detail will be changed when the new feature development will take place. Otherwise, you would know if there is a real need to refactor the brittle modules, or if you can leave them as they are. To say this straight: I think this is a doomed refactoring ...


223

There are other problems Neither code is good, because both basically bloat the code with a debug test case. What if you want to test more things for whatever reason? phoneNumber = DEV_PHONE_NUMBER_WHICH_CAUSED_PROBLEMS_FOR_CUSTOMERS; or phoneNumber = DEV_PHONE_NUMBER_FROM_OTHER_COUNTRY; Do you want to add even more branches? The significant problem is ...


216

Most of your reasons to keep it are utterly irrelevant, put simply. If the code isn't used, throw it away- any benefit involved in keeping it can be trivially derived from source control. At most, leave a comment saying which revision to find it in. Quite simply, the sooner you cut the code, the sooner you don't have to waste time maintaining it, compiling ...


214

Taking the code examples first. You favour: if (isApplicationInProduction(headers)) { phoneNumber = headers.resourceId; } else { phoneNumber = DEV_PHONE_NUMBER; } function isApplicationInProduction(headers) { return _.has(headers, 'resourceId'); } And your boss would write it as: // Take the right resourceId if application is in production ...


170

What is the objective, scientific rationale to favor local variables over instance variables? Scope isn't a binary state, it's a gradient. You can rank these from largest to smallest: Global > Class > Local (method) > Local (code block, e.g. if, for, ...) Edit: what I call a "class scope" is what you mean by "instance variable". To my knowledge, ...


153

The key problem with legacy code is that it has no tests. So you need to add some (and then more...). This in itself would take a lot of work, as @mattnz noted. But the special problem of legacy code is that it was never designed to be testable. So typically it is a huge convoluted mess of spaghetti code, where it is very difficult or downright impossible ...


142

Yes, you should refactor the code before you add the other features. The trouble with comments like these is that they depend on particular circumstances of the environment in which the code base is running. The timeout programmed at a specific point may have been truly necessary when it was programmed. But there are any number of things that might change ...


138

First off, it gets a little wearing that everyone on this site thinks anything written by anyone else is rubbish. Understanding code is difficult, admittedly some poor programming practices make it more difficult, but, for any reasonably complex system understanding the internal structure and idioms used is going to be hard, even if its well written code. ...


136

Should I take the opportunity to just rename the freaking method? Absolutely. That said, if your code has been released as an API, you should also generally leave the misspelled method and have it forward to the correctly named method (marking it Obsolete if your language supports such things).


134

Modifying code to make it more testable has benefits beyond testability. In general, code that is more testable Is easier to maintain, Is easier to reason about, Is more loosely coupled, and Has a better overall design, architecturally.


125

Like whatsisname said, I believe this is case of cargo cult software design. Factories, especially the abstract kind, are only usable when your module creates multiple instances of a class and you want to give user of this module ability to specify what type to create. This requirement is actually quite rare, because most of the time you just need one ...


122

You are doing fine! Creating automated regression tests is often the best thing you can do for making a component refactorable. It may be surprising, but such tests can often be written without the full understanding of what the component does internally, as long as you understand the input and output "interfaces" (in the general meaning of that word). We ...


120

The only problem I see with your current code is the risk of combinatorial explosion as you add more settings, which can be easily be mitigated by structuring the code more like this: if(compressEnable){ data = compress(data); } if(encryptionEnable) { data = encrypt(data); } return data; I'm not aware of any "design pattern" or "idiom" that this could ...


118

Finished IT projects, even faulty ones, are much better than unfinished ones. Unfinished ones can teach you a lot too, but not as much as finished ones. You may not see it now, but you get an enormous amount of value working with even faulty code. My vote goes for finishing and then, maybe, refactoring - if needed. When you start working with more ...


115

Where? On a home page of a Google-scale website, it is not acceptable. Keep the things as quick as possible. In a part of an application which is used by one person once a year, it is perfectly acceptable to sacrifice performance in order to gain code readability. In general, what are the non-functional requirements for the part of the code you're working ...


111

No. There are several reasons why: Variables with meaningful names can make code easier to comprehend. Breaking up complex formulas into smaller steps can make the code easier to read. Caching. Holding references to objects so that they can be used more than once. And so on.


107

Do not ask management for permission to refactor. It's none of their business. You might as well be asking permission to sharpen a pencil. Management doesn't understand refactoring. It's not a business need. Management shouldn't need to understand it. It's not their job. It's yours. Refactoring is a tool you use to satisfy managements needs. Don't ask ...


100

No. Fix it while you're working on it: If you wait to refactor the bit you're working on, you'll forget a lot about it, and have to spend time to get familiar with it again. You won't end up "gold-plating" code that ends up never being used because requirements changed


100

Here's my personal unscientific impression: all three reasons sound like widespread but false cognitive illusions. Sure, the existing code might be wrong. It might also be right. Since the application as a whole seems to have value to you (otherwise you'd simply discard it), in the absence of more specific information you should assume that it is ...


98

Quick ways to get the key points of Working Effectively With Legacy Code Read Michael Feathers' 12-page PDF, written 2 years before the book. Look at Michael Feathers' presentation: 68 slides Listen to a podcast interview with Michael Feathers. E.g. this 30-minute Hanselminutes episode.


87

Feature envy is a term used to describe a situation in which one object gets at the fields of another object in order to perform some sort of computation or make a decision, rather than asking the object to do the computation itself. As a trivial example, consider a class representing a rectangle. The user of the rectangle may need to know its area. The ...


87

... when arguing with one of my collegues, who is going to the point of declaring constants like: private const char SemiColon = ';'; private const char Space = ' '; private const int NumberTen = 10; The argument you need to be making with your colleague isn't about naming a literal space as Space but his poor choice of name for his constants. Let's say ...


85

You and most of the answerers approach this as a communication issue between two colleagues, but I don't really think it is. What you describe sounds more like a horribly broken code review process than anything else. First, you mention that your colleague is second in command and it's expected that he'll review your code. That's just wrong. By definition, ...


84

The LoC in a method is a completely pointless measure. The important things are separation of concerns and code duplication. A method should only do one thing, and that one thing should be expressed in its name. Other things should be left to other methods. The problems arising from code duplication cannot be overestimated (in other words, they are always ...


80

I think most developers find themselves in this position at some point, and I hope that every developer who's felt victimized realizes how frustrating it will be when he or she becomes the senior and feels compelled to clean up code written by juniors. For me, avoiding conflict in this situation comes down to two things: Courtesy. Talking to someone about ...


78

The original code is using member variables like arguments. When he says to minimize the number of arguments, what he really means is to minimize the amount of data that the methods requires in order to function. Putting that data into member variables doesn't improve anything.


76

The Factory pattern vogue stems from an almost-dogmatic belief among coders in "C-style" languages (C/C++, C#, Java) that use of the "new" keyword is bad, and should be avoided at all costs (or at least centralized). This, in turn, comes from an ultra-strict interpretation of the Single Responsibility Principle (the "S" of SOLID), and also of the Dependency ...


71

To answer that, let's take a real world example that happened to me. In C# a library that I maintain, I had the following code: TResult IConsFuncMatcher<T, TResult>.Result() => TryCons(_enumerator) is var simpleMatchData && !simpleMatchData.head.HasValue ? _emptyValue.supplied ? _emptyValue.value : throw ...


70

He's probably right. If the codebase is so monstrous, so gigantically complicated, so difficult to understand... what makes you think you can write something that does the same thing correctly? Generally a big refactoring is the best place to start - start ripping bits out and combining them into reusable chunks; tidy up the code so its easier to view; ...


Only top voted, non community-wiki answers of a minimum length are eligible