4

Yesterday, I removed duplication in a file. The code was performing translations in a grid[i][j]. The duplication was between the code for the i-axis and the code for the j-axis:

For instance, I rewrote:

void redrawGridLine(int iLine);
void redrawGridColumn(int jColumn);

as

enum CoordSelector { I = 0, J = 1 };

//S indicates the direction of the band (I for a line, J for a column)
void redrawGridBand(CoordSelector S, int sBand);

But at the end of the day, my efforts yielded only a 3-lines reduction...

Was it worth it ?

4
  • Possible duplicate of How would you know if you've written readable and easily maintainable code?
    – gnat
    Commented Apr 21, 2017 at 9:31
  • 2
    code size is not what you should be focusing on when refactoring. The key is readability and maintainability are, which are effected by code size, but code size should not be the be-all and end all.
    – user232573
    Commented Apr 21, 2017 at 9:37
  • 2
    That being said, imho the original version was more readable and self-explanatory :-)
    – Scrontch
    Commented Apr 21, 2017 at 12:14
  • 1
    Were the contents of the methods the exact same? Or did you simply have a big switch statement? Commented Apr 21, 2017 at 19:08

4 Answers 4

16

The problem with code duplication is, if you have cases where a change in one place always requires an analogoues change in another place, then you can too easily forget the change at that second place. This is what makes code really hard to maintain, and that is why it often pays off to remove duplicate code even if it saves you zero lines of code in total.

However, I suggest you try not to refactor in a way the readability decreases so much that you or your peer reviewer have doubts if this was really the right decision. In your example, I would replace your enum CoordSelector by something more self-describing like

  enum Direction { Rows = 0, Columns = 1 };

then you can probably omit the longish comment before redrawGridBand.

1
  • I especially second the suggestion on renaming the enum to be descriptive. Commented Apr 21, 2017 at 19:01
10

You should not measure the result in how many lines of code you removed. Some valuable measures are the time you will gain when rewriting the code. If you need to make an adaptation, you will only need to adapt one method. Less time spent when rewriting the code, less time for someone new who is looking at the code, less time spent on bugs because the other method was not adapted...

2

Two observations--

This is the wrong question

Is removing duplication worth it when it doesn't reduce code size?

Doesn't really matter (see At what point is brevity no longer a virtue?). TLDR: Brevity doesn't matter, readability does.

The question you should ask yourself is-- did I reduce the number of code points that would require maintenance if I needed to change something? E.g. if you find a bug, will it be easier to apply the fix? If the code exists in two places, usually it is more work to fix (and to remember to fix it uniformly across the application).

Duplication can actually be better sometimes

Sometimes code duplication is actually better, e.g. if your system has a high number of dependencies and you are trying to improve the ability to deploy and update things independently. For example, if you have two microservices that need the same primitive string operation, it might be better to implement it twice rather than put it in a shared library, so that you can update each microservice independently without worrying about versioning the common library.

2

As other answers have pointed out, there are good reasons to avoid code duplication, even if it is just three lines of code.

However, I would suggest that you keep the functions

void redrawGridLine(int line);
void redrawGridColumn(int column);

Just change their implementations to (I like the suggestion by @DocBrown for the enum):

enum Direction { Rows = 0, Columns = 1 };

void redrawGridLine(int line)
{
   redrawGridBand(Rows, line);
}

void redrawGridColumn(int column)
{
   redrawGridBand(Columns, column);
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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