Java: Code Refactoring/Optimization

Question

Please enlighten me:

Which one do you prefer? Why? [Readability? Memory concern? Some other issues?]

1.

String strSomething1 = someObject.getSomeProperties1();
strSomething1 = doSomeValidation(strSomething1);
String strSomething2 = someObject.getSomeProperties2();
strSomething2 = doSomeValidation(strSomething2);
String strSomeResult = strSomething1 + strSomething2;
someObject.setSomeProperties(strSomeResult);

2.

someObject.setSomeProperties(doSomeValidation(someObject.getSomeProperties1()) + 
                             doSomeValidation(someObject.getSomeProperties2()));

If you would do it some other way, what would that be? Why would you do that way?

Answer 1

Personally, I prefer the second one. It's less cluttered and I don't have to keep track of those temporary variables.

Might change easily with more complex expressions, though.

Answer 2

I'd go with:

String strSomething1 = someObject.getSomeProperties1();
String strSomething2 = someObject.getSomeProperties2();

// clean-up spaces
strSomething1 = removeTrailingSpaces(strSomething1);
strSomething2 = removeTrailingSpaces(strSomething2);

someObject.setSomeProperties(strSomething1 + strSomething2);

My personal preference is to organize by action, rather than sequence. I think it just reads better.

Answer 3

Option 2 for readability. I don't see any memory concerns here if the methods only do what their names indicate. I would be vary with concatenations though. Performance definitely takes a beat with increasing string concats because of the immutability of Java Strings.

Just curious to know, did you really write your own removeTrailingSpaces() method or is it just an example ?

Answer 4

Option one is the way to go in this case. Throw some comments in between the lines so that we actually know whats going on.

The second option is really hard to decipher.

Answer 5

I would probably go in-between:

String strSomething1 = doSomeValidation(someObject.getSomeProperties1());
String strSomething2 = doSomeValidation(someObject.getSomeProperties2());
someObject.setSomeProperties(strSomething1 + strSomething2);

Option #2 seems like a lot to do in one line. It's readable, but takes a little effort to parse. In option #1, each line is very readable and clear in intent, but the verbosity slows me down when I'm going over it. I'd try to balance brevity and clarity as above, with each line representing a simple "sentence" of code.

Answer 6

I prefer the second. You can make it just as readable with a little bit of formatting, without declaring the extra intermediate references.

someObject.setSomeProperties(
    doSomeValidation( someObject.getSomeProperties1() ) + 
    doSomeValidation( someObject.getSomeProperties2() ));

Your method names provide all the explanation needed.

Answer 7

Hi,

for me, it depends on the context and the surrounding code.

[EDIT: does not make any sense, sorry] if it was in method like "setSomeObjectProperties()", I'd prefer variant 2 but perhaps would create a private method "getProperty(String name)" which removes the trailing spaces if removing the spaces is not an important operation [/EDIT]

If validation the properties is an important step of your method, then I'd call the method "setValidatedProperties()" and would prefer a variant of your first suggestion:

validatedProp1 = doValidation(someObject.getSomeProperty1());
validatedProp2 = doValidation(someObject.getSomeProperty2());
someObject.setSomeProperties(validatedProp1, validatedProp2);

If validation is not something important of this method (e.g. there's no point in returning properties which are not validated), I'd try to put the validation-step in "getSomePropertyX()"

Answer 8

I like both Greg and Bill versions, I think I would more naturally write code like Greg's one. One advantage with intermediary variables: it is easier to debug (in the general case).

Answer 9

I try to have one operation per line. The main reason is this:

setX(getX().getY()+getA().getB())

If you have a NPE here, which method returned null? So I like to have intermediate results in some variable which I can see after the code fell into the strong arms of the debugger and without having to restart!