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 a project written in Java that required some repetitive code. Basically the gist of it is that I'm taking one object and copying it into two different objects -- Which I would just use clone() for but it involves moving some exterior mappings.

My question is, in a language like Java, if you are doing a long series of similar actions on two objects, which of the following ways (or perhaps another way) is more desirable: Either grouping by the ACTION being performed...

    UnitStack usStart = new UnitStack(type, (int)Math.ceil(str / 2d), owner);
    UnitStack usStop  = new UnitStack(type, str / 2, owner);

    Map<String, Cloneable> logStart = usStart.getActionLog();
    Map<String, Cloneable> logStop  = usStop.getActionLog();

    logStart.putAll(usOrig.getActionLog());
    logStop .putAll(usOrig.getActionLog());

    MoveFlags mfStop = (MoveFlags) logStop.get("move");
    mfStop = ((mfStop == null) ? new MoveFlags(usStop) : mfStop.clone());
    mfStop.addMoves(b.distanceBetween(begin, end, owner));
    logStop.put("move", mfStop);

    hcStart.setUnit(usStart);
    hcStop .setUnit(usStop);

Or should I group by the object being worked on?

    UnitStack usStart = new UnitStack(type, (int)Math.ceil(str / 2d), owner);
    Map<String, Cloneable> logStart = usStart.getActionLog();
    logStart.putAll(usOrig.getActionLog());
    hcStart.setUnit(usStart);

    UnitStack usStop  = new UnitStack(type, str / 2, owner);
    Map<String, Cloneable> logStop  = usStop.getActionLog();
    logStop.putAll(usOrig.getActionLog());
    hcStop.setUnit(usStop);

    // Reflect that the move took place...
    MoveFlags mfStop = (MoveFlags) logStop.get("move");
    mfStop = ((mfStop == null) ? new MoveFlags(usStop) : mfStop.clone());
    mfStop.addMoves(b.distanceBetween(begin, end, owner));
    logStop.put("move", mfStop);

Obviously I could contrive some way to write a loop, or maybe extract it into a method but in a case like this, I don't really feel like those would be good solutions. (I have removed many things from both of these -- comments mostly, as well as context, since neither are relevant to the question)

share|improve this question
1  
I would definitely extract those four lines into a method. –  kevin cline Jul 7 '13 at 16:41
    
Doing so would only really achieve two lines removed, since I need to initialize both the UnitStack (which would be the logical output of the method) as well as the Map<String, Cloneable>, which would have to be initialized prior to the method call and then mutated by the method as an argument. So it would really only move the problem a little bit. –  scott_fakename Jul 7 '13 at 16:44
    
It's the first time in like almost 20 years of programming experience that I've seen someone ever considering the first approach. –  user61852 Jul 8 '13 at 14:20
    
It makes a certain sense, in that keeping the similar actions together makes it easier to see if you have made some mistake during one phase. –  scott_fakename Jul 8 '13 at 16:27

1 Answer 1

up vote 8 down vote accepted

I agree with Kevin Cline. You ought to extract those four lines in a method.

It's true that the number of lines of code you save is small, but the process gets much more straightforward, you have less clutter, more testability, and less risk of forgetting one line.

The "Don't Repeat Yourself" principle has less to do with saving lines of code (sometimes, strange as it may seem, it can actually increase the number of lines of code) and more with saving brainpower and concentration upon re-reading the sources.

Also, consider that maybe you always want to initialize a UnitStack. If that should be the case, you could add those lines to the constructor (even if it means adding parameters)

UnitStack usStart = new UnitStack(type, (int)Math.ceil(str / 2d), owner);
usStart.init(hcStart, usOrig);

UnitStack usStop  = new UnitStack(type, str / 2, owner);
usStop.init(hcStop, usOrig);

// UnitStack can remember who its owner is
usStop.reflectMove(begin, end);

Otherwise, I'd group the code by the object being worked on. That way you have two similar blocks of code and you can better spot anomalies.

share|improve this answer
1  
I agree. Grouping by object being worked on lets you think at a higher level of abstraction, by letting you think of a group of statements as a single more abstract operation being performed on multiple objects. –  Mihai Danila Jul 7 '13 at 18:45
    
Just a tip, if you're using an IDE (like Eclipse) turning these repetitive lines into a function is super easy using Refactor > Extract method. –  valbaca Jul 7 '13 at 21:20
    
Sure. As I said in the question, I do think extracting to a method is the right approach. But saying to refactor misses the point of the question... There comes a point where you have irreducible complexity. Extracting to a method just requires different repetitions. The question is how to format when you MUST repeat, not "how can I reduce repetition." That being said, at the end, this answer does answer the question so it was accepted. –  scott_fakename Jul 7 '13 at 23:10

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.