Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have an ugly bit of code - essentially iteration over some data structures where the meat of the action was changing, but the iteration code stayed same. The iteration constituted the bulk of code, and there were at least four cases of code copying.

So I refactored the code, putting each level of iteration into its own class (I'll probably do another post on it later).The new code is considerably nicer (at least to my eye), with no copy/paste locations. However, due to some infrastructure I had to introduce to refactor nested loops into separate classes, I ended up with 10% larger number of LOCs. If I discount the extra infrastructure, I get about 5% smaller code compared to the original size.

So, the code is more sophisticated, but it is not shorter and may be harder to understand for a less experinced programmer. I may still end up with shorter code in the future if more oppotunities for reuse present themselves.

My question is: do you think it was worth it? Is it a good idea to refactor for "once and only once" if the LOC count goes up?

share|improve this question
add comment

7 Answers 7

up vote 12 down vote accepted

Lines of code is not a good measure of code complexity. It should only be used as a rough estimate of project size. For example, a 1k LOC program is generally simpler than a 10k LOC program. I'm assuming that that actual number of lines you are talking about stays within the negligible spectrum.

Reducing code repetition is always good because it prevents mistakes later if you need to change something.

share|improve this answer
    
Agree on the usefulness (or lack thereof) of LoC as a code metric. –  ChrisF Mar 25 '11 at 21:23
1  
“Measuring programming progress by lines of code is like measuring aircraft building progress by weight.” - BILL GATES. Still applies to refactoring. It doesn't matter how many lines you have. What matters is readability, maintainability and performance. –  P.Brian.Mackey Mar 25 '11 at 22:06
add comment

Hard to say without seeing the code you refactored, but removing code duplication is always a good thing in my mind, even if it ends up increasing the LOC count.

share|improve this answer
add comment

Slightly longer code can be easier to maintain if it means that a policy change can be implemented in a single place. Collecting a complex nested iteration into a class is a good idea, but in my experience most programmers are not familiar with this pattern.

share|improve this answer
add comment

Repeated code is bad because it's easy for the copies to go out of sync when you have to make a change to how they function. Even if you don't forget to update all the copies when you change one, you're still going to need to make four( or however many copies you have) changes every time you make a change.

And at that point you're likely to say "four copies of the same code? I should refactor this."

share|improve this answer
add comment

Coding style is about communicating intent to people, whether yourself in six months time, or your co-worker on the project, or whoever has your job after your move on.

So if your code is clearer and more understandable, that's a big win. There will be fewer bugs in it, and fewer bugs in code that interacts with it.

I think your point about potential later reuse is a good one as well.

share|improve this answer
add comment

Following DRY (“Don’t Repeat Yourself”) has been worth the effort in my experience. I know when I work on my code that things are specified once and only once, and that simplifies understanding and developing the code.

Functional languages can help with this because you can pass a function into a method that applies the function to every element of a collection, for example.

I was screening a code sample from a job applicant for a client recently and I saw two methods that were identical except for the method names, and the names of one of the identifiers. I knew I needed to look no further at the applicant.

share|improve this answer
add comment

As you describe your problem, I don't understand why each level of the iteration needed to go in a separate class. It sounds like you have something like nested collections, and you need to consider all elements in some certain order, but multiple times. That is, you need one procedure, which takes the top level data structure, and and an operation to perform on each element, and then applies the operation on each element in the prescribed order. If you've changed using bare data types into calling into wrapper classes, but your code still repeats nesting, you haven't gained anything.

share|improve this answer
    
I iterate over two parallel data structures that have same shape but dissimilar implementation. I copy the low level data one to the other. The target structure is larger than the source, so the correct subset of target nodes needs to be identified to match the source nodes. The structures have four kinds of layers, combined two or three deep, according to application usage. I can mix and match my per-layer loops as I need. –  Arkadiy Mar 25 '11 at 22:24
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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