• Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

Java Best Practice - Elegant code - Opinions please?

 
conor o callaghan
Greenhorn
Posts: 5
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
This is just a question of taste. I don't expect everyone to agree with me, but is it just me or does anyone else here find this bit of code a little annoying.

I mean its perfect - it does what its meant to do but does anyone else not think it should be written slightly differently? If so how and why?

 
John Jai
Rancher
Posts: 1776
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Does object changes for each iteration?
 
Mohamed Sanaulla
Saloon Keeper
Pie
Posts: 3159
33
Google App Engine Java Ruby
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
1. What is it that you want the code to achieve?
2. If its just a condition and no fixed number of iterations- you can choose while loop.
But without the context in which this code is used, why this piece of code is written, I dont think much can be told about the code.
 
Winston Gutkowski
Bartender
Pie
Posts: 9501
50
Eclipse IDE Hibernate Ubuntu
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
conor o callaghan wrote:This is just a question of taste. I don't expect everyone to agree with me, but is it just me or does anyone else here find this bit of code a little annoying.

I mean its perfect...

Isn't that a tortological statement?

However, to answer your question, and with nothing else to go by:
1. Your code actually looks more like a while loop than a for.
2. Assuming (1), and that there are no other wrinkles you haven't told us about, you could rewrite it as:Why? Because it's (a) clearer (I happen to hate negative logic), and (b) faster.

HIH

Winston
 
Peter Kirk
Greenhorn
Posts: 12
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'd probably just write it
for (cond) {
if (object.isProcessable()) {
other lines;
}
}

or a while loop as others have said, depending on what the iteration is. But I don't think
while (cond && object.isProcessable())

is neccessarily the same.

 
Rob Spoor
Sheriff
Pie
Posts: 20398
47
Chrome Eclipse IDE Java Windows
  • 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
It isn't. If the continue would be replaced by a break then they would have done the same thing, but the loop shouldn't end if the order is not processable.
 
conor o callaghan
Greenhorn
Posts: 5
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Peter Kirk wrote:I'd probably just write it
for (cond) {
if (object.isProcessable()) {
other lines;
}
}

or a while loop as others have said, depending on what the iteration is. But I don't think
while (cond && object.isProcessable())

is neccessarily the same.



Its a simple for loop with a list of objects

I agree that the following is better

for (DataObject object : objects) {
if (object.isProcessable()) {
other lines;
}
}


I guess I just dont see the sense in. I saw this code and posted it here simply because
I prefer to avoid negated conditions.
A continue is not necessary and it feels like one is almost going out of there way to over complicate the code ( I know its not actually complicated)

I'm totally being anal about it but its not consistent with other existing coding practices. Its different for the sake of being different.
 
Ernest Friedman-Hill
author and iconoclast
Marshal
Pie
Posts: 24204
34
Chrome Eclipse IDE Mac OS X
  • 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'll occasionally use the



format if indenting "something" one more level would be bad, or if "something" is complex and/or expected behavior. Most of the time I'd agree with your rewrite, but I wouldn't say that the original form is always out of place.
 
Peter Kirk
Greenhorn
Posts: 12
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Recently I did actually do something similar, I must admit. I was bug fixing some code, and came across something like this


A problem could occur sometimes in the loop, where "object" could become invalid for further processing. So I just did something like:


so shoot me.
 
conor o callaghan
Greenhorn
Posts: 5
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ernest Friedman-Hill wrote:I'll occasionally use the



format if indenting "something" one more level would be bad, or if "something" is complex and/or expected behavior. Most of the time I'd agree with your rewrite, but I wouldn't say that the original form is always out of place.


yes but if its complex then shouldnt it be,

if( x )
{
doSomething();
}


As I said its a stupid post by me but I was curious as to what others thought. At the end of the day we are all individuals and we have our own preferences, but I guess our individuality can also cause inconsistency in the look and feel of the code. There is no right answers here but more so what is more acceptable... If you were to pick up the code from that person what would most people prefer..

I think we have our answer.
 
conor o callaghan
Greenhorn
Posts: 5
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Peter Kirk wrote:Recently I did actually do something similar, I must admit. I was bug fixing some code, and came across something like this


A problem could occur sometimes in the loop, where "object" could become invalid for further processing. So I just did something like:


so shoot me.


Now in your circumstances I agree - I've done it many times simply because of the depth and other rules within the loop, the easies solution would be to continue or break out of it, but my problem with the code was that it was basic - There was no complexity, it was literally the for loop with about 4 lines of code being avoided by a continue..

There was no complexity no reason to do it in that way.

it was literally something like

for( Job job : jobs)
{
if( !job.isProcessable())
{
continue;
}

simple line of code no condition
simple line of code no condition
simple line of code no condition
simple line of code no condition
}
 
Winston Gutkowski
Bartender
Pie
Posts: 9501
50
Eclipse IDE Hibernate Ubuntu
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Peter Kirk wrote:But I don't think
while (cond && object.isProcessable())
is neccessarily the same...

Actually, based on what we were supplied, it is; but now that conor has told us what the loop is actually doing, I think I'd write it as:
Winston

BTW - there's quite a good article on the lines of this subject here.
 
Peter Kirk
Greenhorn
Posts: 12
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Winston Gutkowski wrote:
Peter Kirk wrote:But I don't think
while (cond && object.isProcessable())
is neccessarily the same...

Actually, based on what we were supplied, it is;


Possibly, but not "neccessarily". As you say, there wasn't entirely enough information if the original post.
 
Joe Areeda
Ranch Hand
Posts: 331
2
Java Netbeans IDE Tomcat Server
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'll add my irrelevant opinion.

I would write it as:

I think

Is different because it stops at the first object that is not Processable and the original code searches the whole list.

I don't particularly object to negative conditions but I dislike continue, break and multiple returns when they are not necessary. It's an old "structured programming" concept that I internalized.
If functions an loops have multiple exit paths I believe they are more error prone.

Joe
 
Winston Gutkowski
Bartender
Pie
Posts: 9501
50
Eclipse IDE Hibernate Ubuntu
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Joe Areeda wrote:I think
Is different because it stops at the first object that is not Processable and the original code searches the whole list.

At the risk of flogging this to death, the code originally posted wasn't clear and I took 'cond' to mean some kind of condition, rather than a for-each. And since the isProcessable() test was the first line in the loop, it seemed logical that it could be put outside.

Of course 'cond' could have included something to retrieve object, so I was wrong anyway; but at least it prompted further explanation .

Winston
 
Joe Areeda
Ranch Hand
Posts: 331
2
Java Netbeans IDE Tomcat Server
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Winston Gutkowski wrote:
Joe Areeda wrote:I think
Is different because it stops at the first object that is not Processable and the original code searches the whole list.

At the risk of flogging this to death, the code originally posted wasn't clear and I took 'cond' to mean some kind of condition, rather than a for-each. And since the isProcessable() test was the first line in the loop, it seemed logical that it could be put outside.

Of course 'cond' could have included something to retrieve object, so I was wrong anyway; but at least it prompted further explanation .

Winston
It is kind of unclear what is going on and there seem to be multiple interpretations so far.

The original code looks to me like a polling situation in a background task, where cond is some kind of switch to terminate the process and object.isProcesable() is somehow changing state external to the loop.

But who know what's really going on and whether the OP is really saturating that process waiting for one of those two thing to change.

Joe
 
Winston Gutkowski
Bartender
Pie
Posts: 9501
50
Eclipse IDE Hibernate Ubuntu
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Joe Areeda wrote:It is kind of unclear what is going on and there seem to be multiple interpretations so far.
Actually OP has now clarified it as being basically a for-each loop.
The original code looks to me like a polling situation in a background task, where cond is some kind of switch to terminate the process and object.isProcesable() is somehow changing state external to the loop.
Wow, that never even occurred to me; but you're quite right. Yet another reason to read that "Tell, don't ask" article .

Winston
 
Wendy Gibbons
Bartender
Posts: 1107
Eclipse IDE Oracle VI Editor
  • 0
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
the reason for the continue maybe becasue the "other code" is quite large, and the writer is trying to avoid too many indentations.
so something like



might be neater.

bother just read more of the thread, so will change it to say ... at one time before refactoring the other code may have been large
 
I agree. Here's the link: http://aspose.com/file-tools
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic