55

I have refactored some code at work lately, and I thought I did a good job. I dropped 980 lines of code to 450 and halved the number of classes.

When showing this to my colleagues some did not agree that this was an improvement.

They said - "fewer lines of code is not necessarily better"

I can see that there could be extreme cases where people write really long lines and/or put everything in a single method to save a few lines, but that is not what I did. The code is in my opinion well structured and simpler to comprehend/maintain due to it being half the size.

I'm struggling to see why anyone would want to work with double the code that is required to get a job done, and I'm wondering if anyone feels the same as my colleagues and can make some good cases for having more code over less?

  • 146
    Code size is measured in time you need to read and understand it, not lines or number of characters. – Bergi Aug 31 '17 at 4:54
  • 14
    Your question as written is categorically Too Broad. Recommend writing a new one about specific changes you made instead. – jpmc26 Aug 31 '17 at 4:57
  • 8
    Consider fast inverse square root algorithm. Implementing full Newton method with proper naming of variables would be much clearer and much easier to read even though it would likely contain more lines of code. (Note that in this particular case using smart code was justifiable by perf concerns). – Maciej Piechotka Aug 31 '17 at 6:23
  • 65
    There is a whole stack exchange site dedicated to answering your question: codegolf.stackexchange.com. :) – Federico Poloni Aug 31 '17 at 7:44
  • 10
    Possible duplicate of At what point is brevity no longer a virtue? – Bob Tway Aug 31 '17 at 8:55
122

A thin person isn't necessarily healthier than an overweight person.

A 980 lines children story is easier to read than a 450 lines physics thesis.

There are many attributes that determine the quality of your code. Some are simply computed, like Cyclomatic Complexity, and Halstead Complexity. Others are more loosely defined, such as cohesion, readability, understandability, extendability, robustness, correctness, self-documentation, cleanliness, testability and many more.

It could be, for example, that while you reduced the overall length of the code - you introduced additional unwarranted complexity and made the code more cryptic.

Splitting a long piece of code into tiny methods could be as harmful as it could be beneficial.

Ask your colleagues to provide you with specific feedback as to why they think your refactoring efforts produced an undesirable result.

| improve this answer | |
  • 1
    @PiersyP just an FYI, one of the guidelines I was taught about a good refactoring is that we should see the cyclomatic complexity reduced to a square root of what it originally was. – M.A. Hanin Aug 31 '17 at 8:54
  • 4
    @PiersyP also, I'm not saying that your code is worse or better than what it was. As an outsider I can't really tell. It could also be that your colleagues are over-conservative and afraid of your change simply because they didn't make the effort required to review and validate it. That is why I suggested you ask them for additional feedback. – M.A. Hanin Aug 31 '17 at 8:56
  • 6
    Good job, guys - you've established that there's a "right" weight somewhere (exact number may vary). Even @Neil 's original posts says "OVERweight" as opposed to just "the heavier a person is", and that's because there IS a sweet spot, just as there is with programming. Adding code beyond that "right size" is just clutter, and removing lines below that point just sacrifices comprehension for the sake of brevity. Knowing where exactly that point is... THAT's the difficult bit. – A C Aug 31 '17 at 14:16
  • 1
    Just because it isn't necessary does not mean it doesn't have value. – Chris Wohlert Aug 31 '17 at 14:39
  • 1
    @Neil You're generally right, but the ever-elusive "balance" you allude to is something of a myth, objectively speaking. Everyone has a different idea of what a "good balance" is. Clearly, OP thought he had done something good, and his coworkers didn't, but I'm sure they all thought to themselves that they had the "right balance" when they wrote the code. – code_dredd Sep 1 '17 at 1:15
35

Interestingly, a colleague and I are currently in the middle of a refactor that will increase the number of classes and functions by a little less than double, although the lines of code will stay around the same. So I happen to have a good example.

In our case, we had one layer of abstraction that really should have been two. Everything was crammed into the ui layer. By splitting it into two layers, everything becomes more cohesive, and testing and maintaining the individual pieces becomes much simpler.

It's not the size of the code that's bothering your colleagues, it's something else. If they can't articulate it, try to look at the code yourself as if you had never seen the old implementation, and evaluate it on its own merits rather than just in comparison. Sometimes when I do a long refactor I sort of lose sight of the original goal and take things too far. Take a critical "big picture" look and put it back on track, maybe with the help of a pair programmer whose advice you value.

| improve this answer | |
  • 1
    Yes, definitely separate UI from other things, this is always worthwhile. On your point about losing sight of the original goal, I agree somewhat, but also you might redesign in to something better, or on the way to better. Like the old argument about Evolution ("what good is part of a wing?") things do not improve if you never take time to improve them. You don't always know where you are going until well on the way. I agree with trying to find out why the coworkers are uneasy, but maybe it really is "their problem", not yours. – user251748 Aug 31 '17 at 12:43
17

A quote, often attributed Albert Einstein, comes to mind:

Make everything as simple as possible, but not simpler.

When you go overboard in trimming things down, it can make the code more difficult to read. As "easy/hard to read" can be a very subjective term, I'll explain exactly what I mean by this: a measure of the degree of difficulty that a skilled developer will have in determining "what does this code do?" by just looking at the source, without the assistance of specialized tools.

Languages like Java and Pascal are infamous for their verbosity. People often point to certain syntactical elements and derisively say that "they're just there to make the compiler's job easier." This is more or less true, except for the "just" part. The more explicit information there is, the easier the code is to read and understand, not only by a compiler but also by a human being.

If I say var x = 2 + 2;, it's immediately obvious that x is supposed to be an integer. But if I say var foo = value.Response;, it's a whole lot less clear what foo represents or what its properties and capabilities are. Even if the compiler can easily infer it, it puts a lot more cognitive effort on a person.

Remember that programs must be written for people to read, and only incidentally for machines to execute. (Ironically, this quote comes from a textbook devoted to a language infamous for being extremely difficult to read!) It's a good idea to remove things that are redundant, but don't take away code that makes it easier for your fellow human beings to figure out what's going on, even if it's not strictly necessary for the program being written.

| improve this answer | |
  • 7
    the var example isn't a particularly good one of simplification because most of the time reading and understanding the code involves figuring out behaviour at a certain level of abstraction, so knowing the the actual types of specific variables typically doesn't change anything (it only helps you understand lower abstractions). A better example would be multiple lines of simple code squashed up into a single convoluted statement - e.g. if ((x = Foo()) != (y = Bar()) && CheckResult(x, y)) takes time to grok, and knowing the types of x or y doesn't help in the slightest. – Ben Cottrell Aug 31 '17 at 6:21
14

Longer code can possibly be easier to read. It's usually the opposite, but there are plenty of exceptions - some of them outlined in other answers.

But let's look from a different angle. We assume the new code will be seen as superior by most skilled programmers who see the 2 pieces of code without having additional knowledge of the company's culture, code base, or roadmap. Even then, there are plenty of reasons to object the new code. For brevity I will call "People critizising the new code" Pecritenc:

  • Stability. If the old code was known to be stable, the new code's stability is unknown. Before the new code can be used it still needs to be tested. If for some reason proper testing isn't available, the change is a rather big problem. Even if testing is available, Pecritenc may think the effort is not worth the (minor) improvement of the code.
  • Performance/scaling. The old code may have scaled better, and Pecritenc assumes that performance will become an issue down the road as clients and features soon* pile up.
  • Extensibility. The old code might have allowed easy introduction of some features that Pecritenc assumes to be added soon*.
  • Familiarity. The old code may have reused patterns that are used in 5 other places of the company's codebase. At the same time the new code uses a fancy pattern that only half the company has ever heard of at this point.
  • Lipstick on a pig. Pecritenc may think both the old and the new code are rubbish, or irrelevant, thus making any comparison between them pointless.
  • Pride. Pecritenc may have been the original author of the code and doesn't like people making massive changes to his code. He might even see improvements as a light insult, because they imply he should have done better.
| improve this answer | |
  • 4
    +1 for 'Pecritenc', and a very nice summary of preasonable objections that should be preconsidered before prefactoring. – user251748 Aug 31 '17 at 12:47
  • 1
    And +1 for 'extensibility' - I was thinking the original code may have had functions or classes that were intended for use in a future project, so the abstractions might have seemed redundant or unnecessary but only in the context of a single program. – Darren Ringer Aug 31 '17 at 13:43
  • Also, the code in question may not be critical code, so considered a waste of engineering resources to clean it up. – Erik Eidt Aug 31 '17 at 15:38
  • @nocomprende Any reason you used preasonable, preconsidered, and prefactoring? Method similar to Pecritenc maybe? – Milind R Sep 1 '17 at 2:23
  • @MilindR Probably a preconception, a predilection, or perhaps a personal preference? Or, maybe just no reason at all, a cosmic confluence of cofactors, confounding conspiratorial conditions. No idea, really. How about you? – user251748 Sep 1 '17 at 11:36
1

What kind of code is better may depend on programmers' expertise and also on the tools they use. For example, here is why what would normally be considered poorly written code may be more effective in some situations than well written object oriented code that makes full use of inheritance:

(1) Some programmers just don't have an intuitive grasp of object oriented programming. If your metaphor for a software project is an electric circuit, then you will expect a lot of code duplication. You will like to see more or less the same methods in many classes. They will make you feel at home. And a project where you have to look up methods in parent classes or even in grandparent classes to see what's going on may feel hostile. You don't want to understand how the parent class works and then understand how the current class differs. You want to understand directly how the current class works, and you find the fact that the information is spread over several files confusing.

Also, when you just want to fix a specific problem in a specific class, you may not like having to think about whether to fix the problem directly in the base class or overwrite the method in your current class of interest. (Without inheritance you wouldn't have to take a conscious decision. The default is to just ignore similar problems in similar classes until they are reported as bugs.) This last aspect is not really a valid argument, though it might explain some of the opposition.

(2) Some programmers use the debugger a lot. Even though in general I am myself firmly on the side of code inheritance and preventing duplication, I share some of the frustration I described in (1) when debugging object oriented code. When you follow code execution, it sometimes keeps jumping around between (ancestor) classes even though it stays in the same object. Also, when setting a breakpoint in well written code it's more likely to trigger when it isn't helpful, so you may have to spend effort on making it conditional (where practical), or even on manually continuing many times before the relevant trigger.

| improve this answer | |
  • 3
    "grandparent classes"! haw haw! Just watch out for the Adam and Eve classes. (And the God class of course) Before that, it was without forms, and void. – user251748 Aug 31 '17 at 12:51
1

It totally depends. I've been working on a project that does not allow boolean variables as function parameters, but instead requires a dedicated enum for each option.

So,

enum OPTION1 { OPTION1_OFF, OPTION1_ON };
enum OPTION2 { OPTION2_OFF, OPTION2_ON };

void doSomething(OPTION1, OPTION2);

is a lot more verbose than

void doSomething(bool, bool);

However,

doSomething(OPTION1_ON, OPTION2_OFF);

is a lot more readable than

doSomething(true, false);

The compiler should generate the same code for both, so there is nothing to be gained by using the shorter form.

| improve this answer | |
0

I'd say cohesion could be a problem.

For example in a web application , Lets say you have an Admin page in which you index all the products , which is essentially the same code (index) as you'd use in a homepage situation , to .. just index the products.

If you decide to partialize everything so you can stay DRY and sleek , you'd have to add a lot of conditions regarding if the user browsing is an admin or not and clutter the code with unnecessary stuff which will make it highly unreadable from let's say a designer!

So in a situation like this even if the code is pretty much the same , just because it could scale to something else and the use cases could slightly change , it would be bad to go after each of them by adding conditions and ifs. So a good strategy would be to ditch the DRY concept and break the code to maintainable parts.

| improve this answer | |
0
  • When less code doesn't do the same job as more code. Refactoring for simplicity is good, but you must take care not to oversimplify the problem space that this solution meets. 980 lines of code might handle more corner cases than 450.

  • When less code doesn't fail as gracefully as more code. I've seen a couple "ref***toring" jobs done on code to remove "unnecessary" try-catch and other error-case handling. The inevitable result was instead of showing a dialog box with a nice message about the error and what the user could do, the app crashed or YSODed.

  • When less code is less maintainable/extensible than more code. Refactoring for conciseness of code often removes "unnecessary" code constructs in the interest of LoC. Trouble is, those code constructs, like parallel interface declarations, extracted methods/subclasses etc are necessary should this code ever need to do more than it currently does, or do it differently. In the extreme, certain solutions custom-tailored to the specific problem may not work at all if the problem definition changes just a little bit.

    One example; you have a list of integers. Each of these integers has a duplicate value in the list, except for one. Your algorithm must find that unpaired value. The general-case solution is to compare every number against every other number until you find a number that has no dupe in the list, which is an N^2-time operation. You could also build a histogram using a hashtable, but that's very space-inefficient. However, you can make it linear-time and constant-space by using a bitwise XOR operation; XOR every integer against a running "total" (starting with zero), and at the end, the running sum will be the value of your unpaired integer. Very elegant. Until the requirements change, and more than one number in the list could be unpaired, or the integers include zero. Now your program either returns garbage or ambiguous results (if it returns zero, does that mean all elements are paired, or that the unpaired element is zero?). Such is the problem of "clever" implementations in real-world programming.

  • When less code is less self-documenting than more code. Being able to read the code itself and determine what it's doing is critical to team development. Giving a brain-f*** algorithm you wrote that performs beautifully to a junior developer and asking him to tweak it to modify the output slightly is not going to get you very far. Plenty of senior devs would have trouble with that situation as well. Being able to understand at any given time what the code is doing, and what could go wrong with it, is key to a working team development environment (and even solo; I guarantee you that the flash of genius you had when you wrote a 5-line method to cure cancer is going to be long gone when you come back to that function looking to make it cure Parkinson's too.)

| improve this answer | |
0

Computer code needs to do a number of things. A "minimalist" code that doesn't do these things isn't good code.

For instance, a computer program should cover all possible (or at the bare minimum, all likely cases). If a piece of code covers only the one "base case" and ignores others, it is not good code, even if it is brief.

Computer code should be "scalable." A cryptic code may work for only one specialized application, while a longer, but more open-ended program may make it easier to add on new applications.

Computer code ought to be clear. As another answerer demonstrated, is possible for a hard-core coder to produce a one line "algorithmic" type function that does the job. But the one-liner had to be broken up into five different "sentences" before it was clear to the average programmer.

| improve this answer | |
  • Duty is in the eye of the beholder. – user251748 Sep 5 '17 at 14:01

Not the answer you're looking for? Browse other questions tagged or ask your own question.