275

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 ...


229

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 ...


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

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 ...


171

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"....


144

It‘s a question of risk management: Refactoring a system always creates the risk of breaking something that worked before. The larger the system, the higher its complexity, and the higher the risk of breaking something. With spaghetti-code (or any other poorly structured code) the real structure of the code remains fuzzy, and the dependencies might be ...


143

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 ...


141

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 ...


137

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).


123

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 ...


119

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 ...


118

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 ...


110

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.


109

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 ...


106

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 ...


99

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 ...


99

One reason is it's really difficult to measure the loss of productivity the messy code is causing, and difficult to estimate the work it will take to clean it properly and fix any regressions. The other reason is many developers incorrectly call any rewriting refactoring. True refactoring is very methodical, almost mechanical. You make very small changes ...


93

... 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 ...


86

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 ...


81

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.


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; ...


70

Last time I tried to start a refactoring with unforeseen consequences, and I could not stabilize the build and / or the tests after one day, I gave up and reverted the codebase to the point before the refactoring. Then, I started to analyze what went wrong and developed a better plan how to do the refactoring in smaller steps. So my advice for avoiding ...


70

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 ...


69

It depends on the context, but I would argue you should declare the most abstract type possible. That way your code will be as general as possible and not depend on irrelevant details. An example would be having a LinkedList and ArrayList which both descend from List. If the code would work equally well with any kind of list then there is no reason to ...


66

A lot of people think that unit testing is method-based; it's not. It should be based around the smallest unit that makes sense. For most things this means the class is what you should be testing as a whole entity. Not individual methods on it. Now obviously you will be calling methods on the class, but you should be thinking of the tests as applying to ...


63

Factory classes are often implemented because they allow the project to follow the SOLID principles more closely. In particular, the interface segregation and dependency inversion principles. Factories and interfaces allow for a lot more long term flexibility. It allows for a more decoupled - and therefore more testable - design. Here is a non-exhaustive ...


60

My question is: should I refactor the code when I encounter such warnings from the authors No, or at least not yet. You imply that the level of automated testing is very low. You need tests before you can refactor with confidence. for now we are no longer able to add any feature without breaking something else Right now you need to focus on increasing ...


58

This may be a cultural thing. In some cultures, admitting that you made a mistake is unheard of, and asking someone to admit to making a mistake is about the rudest thing you can do. If it is that situation, run away. In my experience with very smart people, if you tell them that something they are doing is less than perfect, they will either (1) give you ...


58

There is no "right" or "wrong" answer to this. However, I will offer my opinion based on 36 years of professional experience designing and developing software systems ... There is no such thing as "self-documenting code." Why? Because that claim in completely subjective. Comments are never failures. What is a failure is code that cannot be understood at all ...


58

The 2 years of experience me was you, but more extreme. I'd always create interfaces for every class, I'd apply any design pattern where I was able to, I'd never inject any concrete implementation, trying to write code that could adapt to anything ever. "You're using a database ? Create a repository, and don't forget the interface !!" I'd say. Now ...


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