tags:

views:

299

answers:

14

What are the merits and demerits of the following two code snippets:

return n==0 ? 0 : n==1 ? 1 : fib(n-1) + fib(n-2);

and

 if(n==0)  
    return 0;
 if(n==1)
    return 1;
 return fib(n-1) + fib(n-2);

for calculating the nth letter in the Fibonacci sequence?

Which one would you favour and why?

+7  A: 

The first one is the devil and must be purged with fire.

Cory Petosky
And the second is little better.
Clifford
What is wrong with first one ?
Yassir
@Yassir - it doesn't calculate a fibonacci number
mocj
@mocj - Edited, thanks. Sorry for the silly mistake
Tom
@tom - that's exactly the point, isn't it? It won't be your first mistake when you write your code like that.
Hans Passant
+1  A: 

I'd prefer neither because they are both too slow. Readability should not come at the cost of an exponential explosion in runtime, especially when there exists a simple way that runs in linear time.

I'd do this something like this (pseudo-code):

a = 0;
b = 1;
n.times { a, b = b, a + b; }

In C you'd have to use a temporary variable unfortunately, but the principle is the same.

Mark Byers
Wow, how delightfully on topic, yet completely off topic.
Earlz
Earlz: Hypocrite? http://stackoverflow.com/questions/2274937/code-readability-vs-conciseness/2275002#2275002
Mark Byers
umm? I don't get it.
Earlz
+1  A: 

I prefer the second over the first, mostly for readability.

The second "reads" well - it has the code broken up, so it reads most like English. This will make it easier to understand for many developers.

Personally, I find multiple, chained ternary operations difficult to follow at times.

Also, I personally find "conciseness" to be a poor goal, in most cases. Modern IDEs make "longer" code much more manageable than it used to be. I'm not saying you want to be overly verbose, but trying to be concise often causes an increase in the maintenance effort, in my experience.

Reed Copsey
+6  A: 

I would favour:

return n <= 1 ? n : fib(n-1)+fib(n-2);
Sani Huttunen
n would have to be unsigned, since fib(-1) is undefined
Tom
Indeed; neither of the Tom's suggestions have much merit.
Clifford
Tom: Sure. But yours are even worse. They wouldn't terminate until they've made an overflow on IntXX.MinValue and back to 0. Might even Stack Overflow before then...
Sani Huttunen
+1  A: 

If you're asking for readability, I'd prefer the second option because it doesn't contain the (double!) ternary operator. Usually you're writing code that other people also have to read, and from the second snippet, it's clear at first sight what the function does. In the first snippet though, one has to resolve both ternary operators "in your head" and additionally think about associativity (I'd think about that automatically because parentheses are missing).

But anyway, you could reduce the two if statements to one:

if(n <= 1) return n;
return fib(n-1) + fib(n-2);
AndiDog
A: 

I'd bet there's no difference in the compiled code. I'd at least try to make it a little more readable:

return n==0 ? 0 : ( n==1 ? 1 : fib(n-1) + fib(n+1) );
Jay
I'd try to make your answer more readable as well ;)
Earlz
+4  A: 

I would prefer writing:

if (n == 0) {
    return 0;
}
else if (n == 1) {
    return 1;
}
else {
    return fib(n-1) + fib(n-2);
}

This is very readable code. I don't even like omitting braces as the code is not that readable and when you maintain code that omits braces, you easily make bugs.

Lauri
I agree, in my opinion, brackets should never ever be omitted.
dassouki
I agree, don't omit braces. (Although I prefer the opening brace on a separate line)
Liz Albin
I disagree, please omit braces. Adding braces on one-line if statements is equivalent to adding semicolons to the end of every line in Python.
Cory Petosky
This is just an matter of opinion. The only exception that I allow myself to do is that I can test is parameters are valid like this:int foo(int bar) { if (bar < 0) throw new IllegalArgumentException();}It requires that the if statement really is on-line statement and does not have new line before the throw statement.One of my favorite quotes is from Kent Beck: "I'm not a great programmer, I'm a pretty good programmer with great habits."I have seen programmers to fail to implement working code/fixing bugs because of this just too many times.
Lauri
+2  A: 

Everything in life is a matter of equilibrium. Finding the right compromise between two opposite ends of the spectrum. Optimality is a scoring function that is highly dependent on the evaluator, and the situation, but you should strive for the sweet spot in everything.

Programming is not different. you should evaluate

  • simplicity
  • terseness
  • efficiency
  • practicality
  • artistic freedom of expression
  • time constraints

and find the sweet spot.

Your first construct is clearly powerful and geeky, but definitely not easy to understand.

Stefano Borini
A: 

I prefer the second one in most situations, but there are times where it seems a bit of a waste to not do it in one line. For instance, I'd prefer

text="my stuff_"+id==null ? "default" : id;

to

text="my stuff_";
if(id==null){
  text+="default";
}else{
  text+=id;
}

Note, this also helps with DRY because now if you need to change the name of text then you only change it in one place, compared to 3.

Earlz
A: 

if you use the ?: operator two or three times you will get used to it so i would go with the first

Yassir
A: 

I'd say even more than @Lauri:

if (n == 0) {
    tmp = 0;
}
else if (n == 1) {
    tmp = 1;
}
else {
    tmp = fib(n-1) + fib(n-2);
}
return tmp;

It's good to have just one exit point.

fortran
So you'd prefer to use a temporary variable than having 3 points of return all at the end of a function and all in one place?
Earlz
What's the problem with that? The compiler should be able to optimise that without problems and it helps to write maintainable and debuggable code.
fortran
Introducing a temporary variable strictly for the purpose of restricting yourself to having only one exit point is the opposite of "maintainable" coding.
mocj
yeah, I bet you're a lot smarter than Dijkstra was...
fortran
When did I make such a claim? You can't seriously claim this version is somehow more maintainable than Lauris can you?
mocj
I claim that you don't know what extrapolation is.
fortran
You can claim whatever you like about me, but I notice you still haven't provided any insight into why you believe your example is somehow more maintainable than Lauris.
mocj
Having just one exit point is also good programming style. I don't argue with that. But must not be the only rule to guide you. Sometimes you have to make compromises. In a way my example also has one exit point as all the return statements have been bundled together using if statements.
Lauri
For a small example like this is OK to have as many returns as you want. But in real life, functions tend to grow when requirements change, and at some point this becomes a necessity because you don't want to spend time trying to figure out why that line of code you just added isn't being executed, then to realise that the function returned many lines above in one conditional branch. That's why.
fortran
I guess we'll have to agree to disagree. 3 quick reasons why not to do it: 1. You're adding a new symbol to the function scope, which can easily be confused with other symbols that may have similar names (temp vs. tmp) 2. You can accidentally re-declare the symbol inside of an enclosed scope of the function, hiding the original. 3. It introduces unnecessary nesting and branching.
mocj
1. Agree, the name should be `result` instead of `tmp`. 2. That is improbable, even if it happens, the effect is near to the cause, so it can be tracked in a narrower scope than the whole function. 3. False. Look above again. Where's the unnecessary nesting and branching?
fortran
#2 - It's not improbable, especially in 'real life' long and complicated functions, more so when the original author of the code is not the one who is maintaining it. My point is this - why write code that has the potential to cause problems, however improbable, when you don't have to. #3 - you're half right here - there is no unnecessary branching in your code, however your final tmp = fib(n-1)+fib(n-2) is required to be within the else, Lauri's isn't. In fact you can remove the elses entirely. if(n==0) return 0; if(n==1) return 1; return fib(n-1)+fib(n-2);
mocj
#2 How many variables have your functions? What kind of names do you choose for your variables? Because I've never created a new variable with the name of another in a narrower scope without noticing (if I'm going to effectively use the new variable in the same scope)... #3 Let the micro optimisations to the compiler. If you really think that removing the `else`s will improve the code (in performance or readability), I hope to never have to work with you... :-s
fortran
Me too - finally you've said something we can agree on... Look, all I'm saying is that programmers make mistakes. Every line of code written contains the potential for bugs, either now or in the future through changes - so don't add unnecessary code - bugs cannot exist in code that doesn't exist. So - how many variables? Only the ones necessary and no more. Names? good ones for the given context. I believe shorter and simpler code is more readable, maintainable and is easier to debug - but as I said earlier we'll have to agree to disagree.
mocj
+1  A: 

Of the two, the second is easier to understand at a glance. However, I'd consolidate it as

if (n <= 1) 
  return n;
else
  return fib(n-1) + fib(n-2);

Or, if you're not into multiple returns:

if (n > 1)
  n = fib(n-1) + fib(n-2);
return n;
John Bode
+1  A: 

I often find that indentation can make the multiple-ternary operators a lot more readable:

return n == 0 ? 0 :
       n == 1 ? 1 :
                fib(n-1) + fib(n-2); 
Dan Vinton
A: 

As a generic rule, you should write readable code, which means code which is most readable by the people who will actually read it. Most of the time, this means "yourself, three weeks later". When you write code, the good question is then "will I be able to read and understand it again next month ?".

Apart from that, the first expression is buggy (it uses fib(n+1) instead of fib(n-2)) and both exhibit the exponential explosion which makes Fibonacci sequence a classical tool for teaching some important practical aspects of algorithmics.

Thomas Pornin