[Java] Multiple returns vs single exit

A place to discuss the implementation and style of computer programs.

Moderators: phlip, Moderators General, Prelates

Essah
Posts: 515
Joined: Mon Nov 02, 2009 9:32 pm UTC

[Java] Multiple returns vs single exit

Postby Essah » Tue Oct 23, 2012 7:54 am UTC

I'm a compute science student and our teacher initially taught us to always only have a single return for each method, but have since discovered that it's a subject for debate and some teachers and code handed out actually use multiple returns.

http://programmers.stackexchange.com/questions/118703/where-did-the-notion-of-one-return-only-come-from
There's a discussion about it here but I was wondering what the opinion about it is here?
I find that sometimes making multiple returns makes things easier but at the same time I can see how debugging is easier with a single exit for each method.
What's the opinion around here? Outdated practice or a good coding principle?

also yay my first post in the Coding board here so hi everybody. Suspect it won't be my last. :mrgreen:

User avatar
jaap
Posts: 2085
Joined: Fri Jul 06, 2007 7:06 am UTC
Contact:

Re: [Java] Multiple returns vs single exit

Postby jaap » Tue Oct 23, 2012 8:43 am UTC

My experience is that returns in the middle of long functions make it harder to understand what is going on, or worse, easier to misunderstand what it is doing. If you need a return in the middle of a long function, then you are probably better off splitting off all the code before it and all the code after it into separate smaller functions. Usually those two parts of code do conceptually different things and so can be put into functions with descriptive function names.

I have no problem at all with functions that have returns near the start, where conditions are tested for which the function has nothing to do. If you need a return in the middle of a loop or set of nested loops, then I feel that that block of code should usually be in it its own function, so no code after it (other than a return) and no more than a few lines of setup code before it if necessary.

EvanED
Posts: 4331
Joined: Mon Aug 07, 2006 6:28 am UTC
Location: Madison, WI
Contact:

Re: [Java] Multiple returns vs single exit

Postby EvanED » Tue Oct 23, 2012 1:29 pm UTC

jaap wrote:If you need a return in the middle of a loop or set of nested loops, then I feel that that block of code should usually be in it its own function, so no code after it (other than a return) and no more than a few lines of setup code before it if necessary.

What if those loops are basically doing a search? My gut feeling is that's most of the time you're in that situation anyway.

And in that case, I firmly believe multiple returns are fine. I feel that "Do the search, if you find what you're looking for return it, and if it's not found then return a default value" is easier to understand then "Do the search, if you find what you're looking for than do some action to break out of the loops, then return some variable that you introduced just to store what you've found and return that."

And that situation is one where you can't really refactor anything into its own function.

User avatar
Xeio
Friends, Faidites, Countrymen
Posts: 5098
Joined: Wed Jul 25, 2007 11:12 am UTC
Location: C:\Users\Xeio\
Contact:

Re: [Java] Multiple returns vs single exit

Postby Xeio » Tue Oct 23, 2012 1:32 pm UTC

I much prefer multiple returns, I don't see any reason to declare a local variable to "store" it then have to jump out of any logic/loops just to get to the return. Either way you have to look for either every place a variable is referenced, or every return in the function. The latter is much simpler in nearly every case because return statements are independent, you don't have to worry about what has happened to that local variable (and if you're returning a local, then you declared it for a reason, not just to follow convention).

Usually (depends on the method) I'll have ~2 returns per method. One a the end, a fall-through for null or the default value (zero/true/false, depending on the method). The other the "real" return, if none of the null-checks or other logic (say, a DB call returns no records) fail.

Even being return friendly, it's pretty rare to see a function with more than 2 or 3 returns though. Chances are if you see that the function is getting complicated there is room to refactor (or it's an uncommon case, such as returning from a switch).

User avatar
jaap
Posts: 2085
Joined: Fri Jul 06, 2007 7:06 am UTC
Contact:

Re: [Java] Multiple returns vs single exit

Postby jaap » Tue Oct 23, 2012 1:40 pm UTC

EvanED wrote:
jaap wrote:If you need a return in the middle of a loop or set of nested loops, then I feel that that block of code should usually be in it its own function, so no code after it (other than a return) and no more than a few lines of setup code before it if necessary.

What if those loops are basically doing a search? My gut feeling is that's most of the time you're in that situation anyway.

And in that case, I firmly believe multiple returns are fine. I feel that "Do the search, if you find what you're looking for return it, and if it's not found then return a default value" is easier to understand then "Do the search, if you find what you're looking for than do some action to break out of the loops, then return some variable that you introduced just to store what you've found and return that."

And that situation is one where you can't really refactor anything into its own function.

I think you misunderstand what I wrote, or I didn't write it very clearly.

I mean that if you have a return inside a loop, a case where splitting the before/after parts into their own functions is not possible, then that loop block should be pretty much the only thing in the function, apart from at most a couple of set-up lines before the loop, and a default return after. That way you keep the function short and easy to understand.

EvanED
Posts: 4331
Joined: Mon Aug 07, 2006 6:28 am UTC
Location: Madison, WI
Contact:

Re: [Java] Multiple returns vs single exit

Postby EvanED » Tue Oct 23, 2012 2:40 pm UTC

Yes, I'll agree with that. I read your post as saying that if you have that situation, you should factor the loop into its own function with only one return -- which of course is obnoxious.

User avatar
WanderingLinguist
Posts: 237
Joined: Tue May 22, 2012 5:14 pm UTC
Location: Seoul
Contact:

Re: [Java] Multiple returns vs single exit

Postby WanderingLinguist » Tue Oct 23, 2012 10:27 pm UTC

I think questioning whether multiple returns are "bad" or "good" is kind of missing the point. I'll give your teacher the benefit of the doubt, because I don't know what context this came up in, but unconditionally labeling something that's style-related as "bad" seem suspicious to me.

The important thing is to write code that easy to understand, both for other programmers, and for yourself when you go back to look at it a year or two later.

If using multiple returns harms clarity, don't use them. If it helps clarity, do use them.

Sure, a bunch of random returns scattered throughout the body of a long rambling function can be confusing, but that problem is caused by having a long, rambling function, not having multiple returns.

But I'm also hesitant about the "don't write long functions" rule, because I've seen that taken to an extreme as well. Apparently, some CS teachers are saying "never write functions longer than X lines" or some nonsense like that, so a CS intern I was working with had broken their code into so many tiny functions that it was impossible to grasp what it was doing at a glance. (In this case, it was UI setup code -- there were several buttons in the user interface, and the 2~3 lines of code needed to set up each button had been broken into a separate function that was only ever called in one place).

Blind adherence to rules like "no returns" can make things worse. Imagine if you have rules like that governing how your room is organized. For example, let's say you have a rule, "books only go on the bookshelves". If your bookshelves fill up, and you still blindly follow the rule, you'll be cramming books in sideways into the spaces above other books, and generally leaving things in a mess. But if you take it as a guideline, you might say "Ah, I have a very deep desk... let me get some bookends and put the books I read a lot (reference books, etc.) at the back of my desk," which might be a better way to organize them. The same code for rules about code organization.

There ARE some things that, as a programmer, it's good to be pedantic about. For example, in the majority of cases, it is really bad to use finalizers in Java, and any code that does should be viewed with a certain amount of healthy fear -- and in the really unusual case where you need one (which I've personally never seen) there should be a comment explaining why the exception is being made. These kind of "red flags" do exist, but multiple returns certainly isn't one of them.

The point is to be aware of the state of your code. If you're a new-ish programmer, it's not a bad idea to ask more experienced programmers to look at your code from time to time, and ask them if there's some way to re-factor things to make it cleaner or easier to read. That's the best way to learn whether multiple returns are bad or good.

troyp
Posts: 557
Joined: Thu May 22, 2008 9:20 pm UTC
Location: Lismore, NSW

Re: [Java] Multiple returns vs single exit

Postby troyp » Wed Oct 24, 2012 9:29 pm UTC

The idea of a single exit is mostly just part of Dijkstra's insanely overzealous response to spaghetti code. I agree with what other people have said: multiple returns should be used when they make code clearer and avoided when they obscure it.

In an imperative language, it's often natural to write functions in this style: "if...then you're done; otherwise, continue...". Multiple return values certainly complicate the control flow at the statement level, but as long as it's still easy for a reader to follow, that's not usually a problem.

Insisting on a single exit point in an imperative language often leads to contortions like having a "result" variable whose sole purpose is to act as a surrogate return statement. If you really want a single exit point, it's better to go all the way and use an expression-based (applicative) language. Doing it in an imperative language is trying to shoehorn statement-based code into an applicative pattern.

Having said that, there are a couple of cases where it might be worthwhile to have the restriction:
  1. learning programming: I think if you use an imperative language to teach introductory programming, it may well be best to insist on a single exit point. This (hopefully) prevents students from miring themselves in spaghetti code and helps teach them to structure their code. It also gives them a chance to discover the real use cases for multiple returns.
  2. proofs: I guess if you want to formally prove your code correct, it might be easier with single exit points. This is quite unusual, though. Most of the time, programmers will only want to do some very limited and informal proofs and don't need to arrange their code specifically to be proof-friendly (making it easy to read and understand should be enough).

DaveInsurgent
Posts: 207
Joined: Thu May 19, 2011 4:28 pm UTC
Location: Waterloo, Ontario

Re: [Java] Multiple returns vs single exit

Postby DaveInsurgent » Fri Oct 26, 2012 9:02 pm UTC

Isn't it more than just about returning "a value" from a place? Isn't that the specific imperative case because of resource control?

Consider

Code: Select all

public Foo getFoo(bool someCheck) {
    if (someCheck) {
        return new FooBar();
    } else {
        return new FooBaz();
    }
}


I've never thought the issue was with two "return" statements. Having

Code: Select all

public Foo getFoo(bool someCheck) {
    Foo foo;
    if (someCheck) {
        foo = new FooBar();
    } else {
        foo = new FooBaz();
    }
    return foo;
}


Is "just as bad" because to me what it's really about is the dynamism of the call to getFoo ... rather, you can have a void notifyFoo(someTest) which fires an event to one or more listeners of Foo events... what does that do? I guess I don't know. I've always thought that one should strive to use events and other "non-return" mechanisms in order to achieve program function. So the problem wasn't with the statements, but the dependency of the return ... mechanism... for program behaviour. Does that make sense?

troyp
Posts: 557
Joined: Thu May 22, 2008 9:20 pm UTC
Location: Lismore, NSW

Re: [Java] Multiple returns vs single exit

Postby troyp » Sat Oct 27, 2012 3:38 am UTC

Normally, when someone complains about multiple returns, it's about structural programming principles, a la Dijkstra. The idea is to make the control flow as simple as possible (basically just if and while statements and their variants - although I assume modern structural programmers would at least make an exception for, uh, exceptions).

So they would think your second example was better than your first. And they would probably consider events much worse than either (although I don't really know how structural programming principles are meant to apply to OO or distributed systems).

User avatar
Mat
Posts: 414
Joined: Fri Apr 21, 2006 8:19 pm UTC
Location: London

Re: [Java] Multiple returns vs single exit

Postby Mat » Sat Oct 27, 2012 11:33 am UTC

WanderingLinguist wrote:Sure, a bunch of random returns scattered throughout the body of a long rambling function can be confusing, but that problem is caused by having a long, rambling function, not having multiple returns.

This. I think the main issue is # of branches. Multiple returns is just an indicator that your function is probably too complex.

If you have some hellish 1000 line function with returns scattered through it, it's doing too much and should be refactored, but the same is true of a 1000 line function with a single return.

I do think return/break/continue statements should be used sparingly though. Short concise functions are ideal but sometimes you don't want to make drastic changes and just making the control structure more obvious is a good thing.

But I'm also hesitant about the "don't write long functions" rule, because I've seen that taken to an extreme as well. Apparently, some CS teachers are saying "never write functions longer than X lines" or some nonsense like that, so a CS intern I was working with had broken their code into so many tiny functions that it was impossible to grasp what it was doing at a glance. (In this case, it was UI setup code -- there were several buttons in the user interface, and the 2~3 lines of code needed to set up each button had been broken into a separate function that was only ever called in one place).

If it helps abstraction I'd be fine with that. It's only a problem when the function doesn't have any meaning outside the context where it's used, because then you have to jump around the code just to understand a small part of it.

User avatar
WanderingLinguist
Posts: 237
Joined: Tue May 22, 2012 5:14 pm UTC
Location: Seoul
Contact:

Re: [Java] Multiple returns vs single exit

Postby WanderingLinguist » Sat Oct 27, 2012 1:38 pm UTC

Mat wrote:
WanderingLinguist wrote:But I'm also hesitant about the "don't write long functions" rule, because I've seen that taken to an extreme as well. Apparently, some CS teachers are saying "never write functions longer than X lines" or some nonsense like that, so a CS intern I was working with had broken their code into so many tiny functions that it was impossible to grasp what it was doing at a glance. (In this case, it was UI setup code -- there were several buttons in the user interface, and the 2~3 lines of code needed to set up each button had been broken into a separate function that was only ever called in one place).

If it helps abstraction I'd be fine with that. It's only a problem when the function doesn't have any meaning outside the context where it's used, because then you have to jump around the code just to understand a small part of it.

Exactly. In this case, it was something like:

Code: Select all

void onCreate() {
    makeLabels();
    makeButtons();
}

void makeLabels() {
    makeTitleLabel();
}

void makeTitleLabel() {
    Label titleLabel = new Label(this);
    titleLabel.setText("Save changes?");
    getView().addChildView(titleLabel);
}

void makeButtons() {
    makeYesButton();
    makeNoButton();
    makeCancelButton();
}

void makeYesButton() {
    m_yesButton = new Button(this);
    m_yesButton.setText("Yes");
    getView().addChildView(m_yesButon);
}

void makeNoButton() {
    m_noButton = new Button(this);
    m_noButton.setText("No");
    getView().addChildView(m_noButon);
}

void makeCancelButton() {
    m_cancelButton = new Button(this);
    m_cancelButton.setText("Cancel");
    getView().addChildView(m_cancelButon);
}


In this case, these functions were only ever called from one place: The onCreate() function. So it didn't really do much for abstraction. And the actual code was much more lengthy then this (there were a LOT of UI controls to set up, and the setup code was scattered all over the place). I refactored it to be more like:

Code: Select all

void onCreate() {
    // Window title
    Label titleLabel = new Label(this);
    titleLabel.setText("Save changes?");
    getView().addChildView(titleLabel);

    // Buttons
    m_yesButton = makeButton("Yes");
    m_noButton = makeButton("No");
    m_cancelButton = makeButton("Cancel");
}

Button makeButton( String label ) {
    Button button = new Button(this);
    button.setText(label);
    getView().addChildView(label);
    return button;
}


But at least it was a good learning experience for the intern ;-)

DaveInsurgent
Posts: 207
Joined: Thu May 19, 2011 4:28 pm UTC
Location: Waterloo, Ontario

Re: [Java] Multiple returns vs single exit

Postby DaveInsurgent » Mon Oct 29, 2012 3:02 pm UTC

The first implementation is superior to yours. You owe the intern an apology.

Abstraction is not the ruling principle when it comes to internal code - readability is.

And the actual code was much more lengthy then this (there were a LOT of UI controls to set up, and the setup code was scattered all over the place)


That doesn't make sense, because you said:

from one place: The onCreate() function


onCreate can call createTitlePane (and that can call createTitleLabel and createTitleLogo) and then call createMessagePane and then createButtonPane and that's easy to debug and maintain: When someone says "We need the message pane to contain an image along with the message" it's easy to understand where that happens. Anyone can jump in and look at onCreate and follow, logically, where the program goes and know which functions are unrelated to the feature/bug.

The one thing you did well is the createButton function - makeYesButton, makeNoButton and makeCancel button could have been refactored to use a single createButton(label) function. The important part is it would reduce code duplication, not lines of code or length. Your refactoring accomplished this only as a side-effect, not as an intent.

Also, you introduced comments. You took perfectly good, self-documenting code and introduced another language in to the source file which now needs to be maintained along with any source changes.

User avatar
Xeio
Friends, Faidites, Countrymen
Posts: 5098
Joined: Wed Jul 25, 2007 11:12 am UTC
Location: C:\Users\Xeio\
Contact:

Re: [Java] Multiple returns vs single exit

Postby Xeio » Mon Oct 29, 2012 3:14 pm UTC

DaveInsurgent wrote:The first implementation is superior to yours. You owe the intern an apology.
I would much rather run into the second, so I'd disagree with you on that point. There is probably a point at which making a function explicitly to set up the buttons or other control types is better, but I don't think it's at 3 lines of code, and not in an 8 line function (including the comments).

DaveInsurgent
Posts: 207
Joined: Thu May 19, 2011 4:28 pm UTC
Location: Waterloo, Ontario

Re: [Java] Multiple returns vs single exit

Postby DaveInsurgent » Mon Oct 29, 2012 3:32 pm UTC

He specifically said that the actual code is much larger.

Also, you seem to be suggesting that at some point, the complexity of the class will reach a point where yes, it's suitable to converge on the "descriptively named" paradigm. If so, then all this really accomplishes is that you have more than one convention for how you structure and format classes, which is obviously inferior in terms of organization and maintainability.

If you receive a task to change the size and position of a button, it's incredibly easy to approach the system from the top-down (as it should be approached):

- the form is controlled by a class
- on creation (onCreate) the dialogue is populated
- it is populated with a title area and a button area
- the button area consists of three buttons
- (insert specific button code here).

This is all done with function names which both serve as the code and the comment. In the second case, you're going to scan a block of text that doesn't fit on your screen, then look for comments that indicate which button is being constructed. Assuming those comments are correct (hint: they probably aren't) you then need to re-read the code anyway to see where you need to make the change.

The form-button situation is a contrived example, obviously. Nobody really cares how something as simple as a dialogue is set up, because it's inherently trivial to maintain. But the intern was attempting to exercise solid, well-founded engineering discipline in how they approached the construction, and the refactoring basically shit all over that. If we focus specifically on the simple form scenario, then it's obvious that the correct thing is to leave whatever is to be there: refactoring it only costs the company money, and doesn't dramatically improve the quality of the software. So all that was accomplished was a bit of ego-programming, discouraging a person from practicing good habits, and company time was wasted. That's the exact opposite of what software engineering is intended to do on all three accounts.

User avatar
WanderingLinguist
Posts: 237
Joined: Tue May 22, 2012 5:14 pm UTC
Location: Seoul
Contact:

Re: [Java] Multiple returns vs single exit

Postby WanderingLinguist » Mon Oct 29, 2012 3:58 pm UTC

The problem here is that the title wasn't in a separate frame. It was in the makeLabels() function. "Label" in this case being the name of the View subclass. In other words, it was organized by the class of object that was being created rather than the role that played in the layout. So, for example, if you wanted to add an icon next to the title text, the only place to add it would've been at the top level in onCreate() anyway.

That's why I have a problem with strict rules about this kind of thing. Each situation is different. In particular, this is an Android app, so adding a frame would mean needing to choose between LinearLayout, RelativeLayout, etc. and adding a whole lot more complexity just in case it needed to be extended later. The point being, the added complexity was in the wrong place.

And yes, the original code was much longer. There were about 20 or so buttons that fit into a few categories, laid out with spacers between them, that were all *identical* except for the labels and member variables they were stored in (even handling code was separate). The result was probably 10 screenfulls (at my monitor's resolution) of 1~3 line functions that could all be fit into a single screenful of code after refactoring.

Really, do you think I did it the wrong way?

The critical thing on a large project is to know where the components should be broken apart, and below that level, to keep the code as easy to read at a glance as possible. That requires not just blindly applying rules, but having some amount of common sense about it too.

You can argue all you want over commenting style and so on (and that could easily become a religious wars topic) but the point is, if you can look at a screenful of code and figure out at a glance what it does, you're in much better shape than having spaghetti code for the sake of "good architecture".

So my objection is CS teachers who say things like "Thou Shalt Never Write Functions More than X Lines" as a hard rule. As a guideline, it's fine. Hard rules should be reserved for things like "Don't name variables in your mission-critical accounting app after Tintin characters" and "Don't use delete to free something you malloc'd". I have no problem with "you really should follow this guideline unless you can think of a really good reason not to", but to represent it as "functions more than 100 lines are as bad as deleting something you malloc'd" is definitely uncool.

I've also seen projects where SO much focus has been spent on engineering the perfect architecture that the app never got finished. Good engineering is critical, of course, but common sense is too.

Edit: italics fail

DaveInsurgent
Posts: 207
Joined: Thu May 19, 2011 4:28 pm UTC
Location: Waterloo, Ontario

Re: [Java] Multiple returns vs single exit

Postby DaveInsurgent » Mon Oct 29, 2012 4:33 pm UTC

and yes, the original code was much longer. There were about 20 or so buttons that fit into a few categories, laid out with spacers between them, that were all *identical* except for the labels and member variables they were stored in (even handling code was separate). The result was probably 10 screenfulls (at my monitor's resolution) of 1~3 line functions that could all be fit into a single screenful of code after refactoring.

Really, do you think I did it the wrong way?


My criticism is twofold:

1. You changed the code because of preference. This is a waste of time and money.
2. You did some good but some bad, meaning you wasted time and money for a net neutral change.

The 1-3 line functions could have been summed up as you did (like I already said) which would have reduced the code size and the code duplication. To me, that's a win-win: but it's also not something worth refactoring by itself. That's something you boyscout in when you make some other change. (Maybe this is what you did, I'm not making that an outright criticism, just saying how I would do it and how I think most companies expect their employees to use their time).


The critical thing on a large project is to know where the components should be broken apart, and below that level, to keep the code as easy to read at a glance as possible. That requires not just blindly applying rules, but having some amount of common sense about it too.


I agree with this, and I understand the subjectivity (not to mention the indirection caused by the fact we're not talking about the same literal piece of code).

If the code is literally:

Code: Select all

void createYesButton {
    yesButton = createButton("yes");
}


Then sure, you've reached a level where the encapsulating "documentation code" completely replicates the "implementation code", in which case you've achieved a kind of code duplication, in which case DRY over rules. But that doesn't mean you can't factor out this:

Code: Select all

void createButtons() {
    yesButton = createButton("Yes");
    noButton = createButton("No");
    cancelButton = createButton("Cancel");
}


You said they're organized by a divider of some kind, so assuming they're grouped together in some way that can be expressed, it's good to have those in separate create[x]ButtonGroup methods.

You can argue all you want over commenting style and so on (and that could easily become a religious wars topic) but the point is, if you can look at a screenful of code and figure out at a glance what it does, you're in much better shape than having spaghetti code for the sake of "good architecture".


I agree about the screenful part. I have never, ever, not once, read a single piece of literature that led the reader in to any kind of spaghetti code situation in the name of good architecture. In fact, in the real world, every single plate of spaghetti code I've ever been served has come from someone who eschews engineering discipline under the guise of pragmatism and exceptional circumstances (which is just another way of saying "not understanding the problem").

So my objection is CS teachers who say things like...


I've never studied CS, all I have to reference are the books I read. I've never encountered anything like what you're suggesting, but of course all published works carry a sort of "confidence" with them, that is not to be construed as "thou shalt" type nonsense. This sounds like yet another reason to be glad I learned to do this on my own.

but to represent it as "functions more than 100 lines are as bad as deleting something you malloc'd" is definitely uncool.


They don't need to be compared. The later is an error. The former is something that, while debatable, also has a reasonably strong level of reason surrounding it from various sources. They don't always agree - but that's no different than any other engineering discipline, none of which are as young as ours. Try to get two people to agree on whether you need to bridge or brace (or both) a pair of joists. You'll have all sorts of "proof" either way. Our profession is maturing and, in the case of comments, there is a lot if evidence now to show that they're to be avoided. The same with "large" functions. You're always going to encounter people who take those things to the absolute: that's because they are in over their heads and aren't actually cut out for the profession.

So, you may have had good reason to refactor the code. If what you did was bring things together in their logical grouping, then that's great. I'd say you still put a bit of "bad' in with your "good" by throwing in comments and, going based only on your cursory example, throwing a lot of crap in to the onCreate method. (This goes back to the "single screen" point you made above: does it fit on a single screen now? If not , then what did you really improve?). You have to be able to defend your refactoring as a "I had to do this instead of work on a feature because ... " and if the only part of that is "I prefer to look at it [my way]", then that's not really a justification that the business can respect. You may get away with it, you may not.

User avatar
Xeio
Friends, Faidites, Countrymen
Posts: 5098
Joined: Wed Jul 25, 2007 11:12 am UTC
Location: C:\Users\Xeio\
Contact:

Re: [Java] Multiple returns vs single exit

Postby Xeio » Mon Oct 29, 2012 4:58 pm UTC

DaveInsurgent wrote:You have to be able to defend your refactoring as a "I had to do this instead of work on a feature because ... " and if the only part of that is "I prefer to look at it [my way]", then that's not really a justification that the business can respect. You may get away with it, you may not.
If this was part of a code review, it's likely that this time was already allocated to bring the intern's code up to standard with the rest of the codebase. I didn't get the impression that this was going into existing code explicitly for the purpose of refactoring.

Modifying existing code is always hard to justify, even if it really should be done, and the code is difficult to read or is otherwise anti-pattern. There's always other things to work on, rather than "fixing" working code (especially if the person you're trying to justify it to isn't or wasn't a coder in their professional history).

User avatar
WanderingLinguist
Posts: 237
Joined: Tue May 22, 2012 5:14 pm UTC
Location: Seoul
Contact:

Re: [Java] Multiple returns vs single exit

Postby WanderingLinguist » Tue Oct 30, 2012 4:38 am UTC

DaveInsurgent wrote:My criticism is twofold:

1. You changed the code because of preference. This is a waste of time and money.
2. You did some good but some bad, meaning you wasted time and money for a net neutral change.


I'd say going from 10 screens to 1 screen is a big improvement. We have to adjust this code a lot as UI requirements change, and the intern in question was struggling with maintaining it (now they can adjust things in a few minutes vs. the few hours it was taking before, so I think it works).

just saying how I would do it and how I think most companies expect their employees to use their time.

Ah, I work for a Korean company, so I suspect things are a bit different here. For one thing, everyone is salaried without exception, so as long as we hit targets there's no micro-managing of time use. Also, no overtime pay,so if we put in extra hours on something like this, nobody cares (unless we stay in the office past 1am or so when the security guys normally get off work).

I agree with this, and I understand the subjectivity (not to mention the indirection caused by the fact we're not talking about the same literal piece of code).

If the code is literally:

Code: Select all

void createYesButton {
    yesButton = createButton("yes");
}


Then sure, you've reached a level where the encapsulating "documentation code" completely replicates the "implementation code", in which case you've achieved a kind of code duplication, in which case DRY over rules.

So my objection is CS teachers who say things like...


I've never studied CS, all I have to reference are the books I read. I've never encountered anything like what you're suggesting, but of course all published works carry a sort of "confidence" with them, that is not to be construed as "thou shalt" type nonsense. This sounds like yet another reason to be glad I learned to do this on my own.



Actually, it sounds like we're in the same boat. I never took CS in university. I should say; I have no problem with the reference books for the most part (most of them have good examples of exceptional cases).

but to represent it as "functions more than 100 lines are as bad as deleting something you malloc'd" is definitely uncool.


They don't need to be compared. The later is an error. The former is something that, while debatable, also has a reasonably strong level of reason surrounding it from various sources.

Exactly this. They aren't the same thing at all, yet apparently (I say apparently, since I didn't study CS in university, as I've mentioned) "functions more than X lines" are represented as being as bad as an error. I'm just saying, throughout my career I've seen new engineers who have written functioning code struggling to refactor it into some other form (sometimes better, sometimes worse) before turning it in just because of some silly rule like that.

Keeping functions short is definitely a good idea. But the goal should be "readable code" not "short functions" (short functions are a tool towards achieving readable code, not an end in and of themselves).

I suspect we kind of are thinking the same way here... merely the business constraints (how time is spent, etc.) being different. The last thing I want to do is refactor just for my own ego (that's incredibly discouraging for interns and people just getting into the field -- I know that from my own experience). The goal here was to help the intern in question get the code into a form that they could manage themselves (previously, they were struggling with it a lot and asking for help). Probably, I should have mentioned that in the beginning.

Edit:

This goes back to the "single screen" point you made above: does it fit on a single screen now?

Yes, it does. That was the point (from 10 screens-ish to about one screen). That wasn't entirely due to the bad structure; there was a small amount of unused code hidden away in there too (which was hard to spot due to the way it was laid out).

(and yes, this was freshly written code that had just been turned in and wasn't even in source control yet; it wasn't something that had been sitting in the code base for 2 years)

DaveInsurgent
Posts: 207
Joined: Thu May 19, 2011 4:28 pm UTC
Location: Waterloo, Ontario

Re: [Java] Multiple returns vs single exit

Postby DaveInsurgent » Tue Oct 30, 2012 3:43 pm UTC

How was the intern having difficulty maintaining something that had not been committed?

I'd love to see the actual code. Doubt that can happen.

Also, I too am salaried and without overtime - as I think most are. But the company still perceives any of your time spent as something that could produce value to the company, so rewriting code just to rewrite it (without tangible proof that it improved things). That is to say - there's always a backlog of features that need to be done.

User avatar
WanderingLinguist
Posts: 237
Joined: Tue May 22, 2012 5:14 pm UTC
Location: Seoul
Contact:

Re: [Java] Multiple returns vs single exit

Postby WanderingLinguist » Tue Oct 30, 2012 10:33 pm UTC

DaveInsurgent wrote:How was the intern having difficulty maintaining something that had not been committed?

We don't normally allow interns to commit directly to source control; they usually have a lot of skills to pick up beforehand, and the first few iterations of code aren't anywhere near production quality. Usually, they get a self-contained component to work on by themselves, and if we're satisfied it's up to the standards of our code base, it gets committed. Sometimes that doesn't happen; sometimes they write excellent code right off the bat.

In this case, they were working on a UI module well ahead of the schedule where we actually needed it (another engineer would have started on it a few week later based on the original schedule). The intern's code was actually pretty good except for the UI creation code, so the engineer who normally would've done it was able to go work on something else instead. But they were having trouble responding to feedback about the UI itself ("move the button here", "add a button to do X") because the setup code was a mess. (The event handling and everything else post-setup were in decent enough shape). So the point was (at their request) to help them refactor the code into a form they code manage.

The point is the intern is there to learn (if they didn't need to, they'll just be a regular employee, not an intern), so if they say "hey, this part of my code has gotten out of hand, help me improve it", well...

I'd love to see the actual code. Doubt that can happen.


For obvious reasons, I can't post it.

Also, I too am salaried and without overtime - as I think most are. But the company still perceives any of your time spent as something that could produce value to the company, so rewriting code just to rewrite it (without tangible proof that it improved things). That is to say - there's always a backlog of features that need to be done.


Well, again, it depends on the amount of time that needs to be spent. As I mentioned, this wasn't a particularly time-consuming task. Sure, refactoring is not something you do on a whim, but at the same time, we are expected to devote a certain amount of our time to working with the interns (nearly all of the senior engineers are assigned as a mentor at least once or twice a year).

Anyway, it comes back to my original point: If you're a new programmer, getting advice from experienced programmers (i.e. asking for code review) is probably one of the better ways to learn about good style. A lot of rules-of-thumb like "one point of return from a function" or "functions no longer than a screenful" are very useful starting points, but you have to know HOW to apply them without making the code worse.

Just as an example, a function that contains some very large lookup table may legitimately be longer than a screenful, but still easy enough to read at a glance. The "no longer than a screenful" rule when applied to such a function might mean something like:
  • Replace the lookup table with code that generates the lookup table the first time. (Great for lookup tables for things like premultiplied alpha, etc.) The new code fits in a screenful, and more usefully shows how the table was generated.
  • Split the lookup table function into its own file (maybe with a wrapper function)
  • etc.
But it doesn't mean arbitrarily splitting it into functions like lookupTablePart1(), lookupTablePart2(), and so on, on arbitrary boundaries. And then there are cases where you simply can't generate the lookup table the first time (for example, if you have nowhere to cache it). A great example of this is a noise function in GLSL where you don't have control of the external C code, so you can't upload a texture to use as a lookup table.

My point being (and admittedly, my over-simplfied code example was lacking) that simply learning rules like "Thou Shalt Never Have Multiple Returns in a Function", without having a deeper understanding of the reasoning behind it and possible exceptions, isn't going to lead to better code. Working with other people's code and having to figure it out WILL help you improve your own code and will lead to a deeper understanding of how to apply rules like "no more than a screenful" and "no multiple returns" in a reasonable way.

Getting someone to review your code in the beginning and help you improve readability is a good thing. Keeping in mind, of course, that people have different ideas of readability, and even experienced programmers may break the rules when they shouldn't in the interest of releasing sometime on time. Still, if you know your code is a tangled mess and need help getting it into a form you (and other programmers) stand a decent chance of managing, getting a more experienced pair of eyes to look at it can be a good help and a good learning experience, as in the case with the intern I mentioned.

Heck, even as a relatively experienced programmer, I'm always asking colleagues to take a quick look at code I'm working on. Asking "Can you think of a simpler way to do this?" while it's still early enough to change things is very good. Some companies have mandatory code review, but for those that don't, I think experienced programmers should always be asking for feedback (within reason). After all, things that seem obvious and self-documenting while we're writing them might NOT seem so obvious a few years down the road. If my colleague doesn't understand my code at a glance, chances are I won't either in a few days (I have a pretty crappy memory).

OrangeLemonade
Posts: 51
Joined: Mon Nov 12, 2012 11:15 pm UTC

Re: [Java] Multiple returns vs single exit

Postby OrangeLemonade » Mon Nov 19, 2012 10:45 pm UTC

I think that the point of teaching new programmers prescriptive rules like "one return per function" is to avoid various kinds of confusions that get created when they break those rules.

When you know what you're doing, you know when to break the rules, but there's a period when you don't know better and spaghetti code (or worse) tends to result without them.


-O


Return to “Coding”

Who is online

Users browsing this forum: No registered users and 3 guests