Please refactor these 6 java lines

[Myke Enriq]

// code that sets a String named randomString                       
long resultLong = -1;
try {
	resultLong = Long.parseLong(randomString);
} catch (Exception e) {
	log.error("Error parsing resultLong : ", e);
}

if (resultLong < 0) {
	return false;
}
// other code that uses resultLong and it assumes that it is >=0

Any ideas on how to improve this code's readability ?

[Dieter Quickfend]

if(longValue(randomString) < 0L) return false;

public Long longValue(final String randomString){
    try{
        return Long.valueOf(randomString);
    }catch(NumberFormatException nfe){
        log.error("Error parsing resultLong : ", e);  
        return -1L;
    }
}

[Tim Cooke]

Any safe refactor depends on having good tests to verify that you do not change the behaviour of the code.

Perhaps you can post the entire method and include any tests you have written for it? I can derive that your method returns boolean due to the "return false" on line 10 but that's about it.

[Joanne Neal]

Dieter Quickfend wrote:

if(longValue(randomString) < 0L) return false;

public Long longValue(final String randomString){
    try{
        return Long.valueOf(randomString);
    }catch(NumberFormatException nfe){
        log.error("Error parsing resultLong : ", e);  
        return -1L;
    }
}

You missed what it said on line 12 of the original post.

[Dieter Quickfend]

Joanne Neal wrote:
You missed what it said on line 12 of the original post.

I would assume that it would be no great feat to use longValue(randomString) in the rest of the method.

[Stephan van Hulst]

After saving it to a local variable of course ;)

[Myke Enriq]

Thank you for your replies. I feel these exercises may prove useful both for me and others. Here is another piece of code:

if (array[index] != null) {
	customObject.setStringField(array[index].toString());
}
index++;

This is from a function that has multiple similar blocks of code like this one, one after another. Basically an object has many fields, and I need to set each of those values from an array of String.

Could I write this easier to read/more elegant ?

[Tim Cooke]

Hi Myke, It's best if we keep it down to one discussion point per thread so feel free to start a new thread with your new question. Otherwise this will become an endless bucket for random refactor efforts.

[Myke Enriq]

Shouldn't we have a topic only for refactoring ?

This could be useful as it will improve a lot of people's coding style.

[Tim Cooke]

Far too generic. We'll end up with a huge single topic with multiple refactoring discussions in it. How would that be useful? How would you search it? How would we talk about it? "Oh that particular refactoring was discussed in the 'Refactoring' topic on page 74". It's just not manageable at all. It's akin to saying "Shouldn't we put all our code in main? It's all just code after all right?"

I agree that discussing refactoring is a valuable thing to do but we should do it one problem at a time, one topic at a time.

[Myke Enriq]

Tim Cooke wrote:Far too generic. We'll end up with a huge single topic with multiple refactoring discussions in it. How would that be useful? How would you search it? How would we talk about it? "Oh that particular refactoring was discussed in the 'Refactoring' topic on page 74". It's just not manageable at all. It's akin to saying "Shouldn't we put all our code in main? It's all just code after all right?"

I agree that discussing refactoring is a valuable thing to do but we should do it one problem at a time, one topic at a time.

I like the way you think, and indeed this has the potential to turn into an epic thread. Would it not be better though if all refactoring discussions happen in a single thread just like you described ? I mean, a novice programmer could just browse through conveniently because it is all in one place.

[Matthew Brown]

I can't speak for anybody else, but once a thread reaches a certain size I wouldn't start reading it because I'd assume it would be too much effort to get up to speed with the discussion. You're also going to get confusion when discussions of different problems start interleaving.

What I'd suggest is using one thread per discussion. And if there were enough of these threads, or if we decided we wanted to encourage more of them, then a new forum could be created specially for them. That would meet the requirement for someone to find them all in one place.