Simple Implementation Comparison

[Ritter688]

Hi folks!

I just did an assignment for my INTRO to programming course, and I got hammered on my first assignment for 'poor programming style'. I'm going to go in and speak with my TA on Friday, and I am in the process of preparing my defense...

So my question is this... I'll show two implementations of a few lines of code, and I want to know if after compilation, one is really more efficient than the other.

1.

  for (i=0; i<TERMS; i++)
  {
    divisor += 2.0;  // Increment divisor.
    switch (add)
    {
      case true:  total += (1/divisor); break;
      case false: total -= (1/divisor); break;
    }
    add = !add; // Flip mathematical operation for next loop increment.
  }

2.

    for (long i = 1; i <= n; i++) {
        sum += sgn * (1.0 / k);
        sgn = -sgn;
        k += 2;
    }

I'm banking on the fact that A) my code snippet (1) is more readable than the TA's code snippet (2), and B) any modern compiler would most likely execute the two code snippets in an equal fashion.

Thanks in advance!

[kooth]

Man is THIS a holy war ;)

I have my own opinions about code formatting style, but the general consensus is to pick a style and stay consistent with that style.

I like lots of space around operators -- to me it's just easier to read. So I like the TA's "for loop" formatting better than yours, but lots of people like the way you have your spacing.

We have a standard to never put more than one statement on a line. The main reason for that is it's hard to set a break point in the debugger when the code is layed out like your case statement.

Personally I like my braces on its own separate line indented to the level of the code, but again, that is a religious issue ...

Any compiler should handle whatever formatting style you use.

Good luck!

[helios]

I believe #2 is faster. Jumping inside a tight loop in modern CPUs is a really bad idea because the processor has to discard the pipeline. I done some optimizations that took advantage of this and gained significant performance boosts. I don't remember the exact amount, but it might have been between 50-100% increases.
I can spot a useless operation in #2, anyway: sum += sgn / k;. Gotta shave those cycles.

But damn! This is for an introductory course?

[Athar]

You're right, your variant is more readable and it is unlikely that there are any major differences in performance (although a quick test in g++ indicates that your variant is about 5% faster - that won't work as an argument, though).
However, the use of switch is quite inappropriate here - that's what if is for.

if (add)total += 1/divisor;
else    total -= 1/divisor;

Another problem is that you have some code duplication (1/divisor). Of course it's negligible in this case, but in general, you'll want to avoid code duplication as much as possible. And the counter variable should be declared inside the for statement, just like your instructor did (unless you need to know the value the loop stopped at - but that is not the case here and you know it anyway (TERMS-1/n)).

I guess it's obvious that your instructor has a mathematical background - his solution is closer to how you would express the sum in mathematical terms, so it's understandable he considers it to be the more "natural" solution.

[Ritter688]

I like lots of space around operators -- to me it's just easier to read. So I like the TA's "for loop" formatting better than yours, but lots of people like the way you have your spacing.

Yeah, the spaces do make it a little clearer, but I've been doing it this way for years...

Any compiler should handle whatever formatting style you use.

I thought they would compile equivalently...

Jumping inside a tight loop in modern CPUs is a really bad idea

Can you explain what you mean by this Helios?

[Athar]

I can spot a useless operation in #2, anyway: sum += sgn / k;. Gotta shave those cycles.

This does simplify the code, however it shouldn't have an impact on performance. g++ automatically does the optimization and produces identical code in both cases.

[helios]

Modern CPUs are designed with instruction level parallelism in mind. That is, an instruction is divided into several micro tasks and ran alongside a few other instructions at the same time, all using different parts of the CPU. This improves performance and power efficiency.
http://en.wikipedia.org/wiki/Instruction_pipeline
When the CPU encounters a conditional jump (e.g. an if, a loop, a switch), it can do one of two things: it can assume the jump will not occur and continue filling the pipeline where the control flow was already going, or it can assume it will occur and continue where it would go if it did occur. It's easier to implement the logic that assumes that the jump will not occur, so that's what the CPU assumes. If the assumption is wrong, the pipeline has to be entirely discarded, because the intermediate results currently in it are based on an incorrect assumption. Once the jump has occurred, the CPU begins filling the pipeline again. Discarding the pipeline is very inefficient because until it is refilled, nothing is running in parallel, and wasteful because the CPU ends up doing work that it has to later throw away.

[kempofighter]

Example 1 would violate every professional coding standard that I have ever seen for a couple of reasons. First you don't normally use a switch for a simple boolean condition. Second you don't normally put three statements on a single line. That certainly doesn't help the readability in any way (at least not in my opinion). I found the second example to be much easier to read and would be compliant with most (probably all) professional coding standards. Athar's alternative if..else would still violate my project's coding standards and still looks kind of funny.

Anyway I hope that you don't end up banking too much on your assumptions because you will probably not win the argument. Good luck!

[Ritter688]

Helios: Thanks for the input, I new that already, just didn't click in. The loop is for 10000 terms, so it's not overly tight, but I get what you're saying about the conditional.

Athar: Appreciate the testing! Nice to know that mine is faster. lol

Kempo & Anyone Else: Is there a document available somewhere that goes over professional coding standards? I'm not really new to programming, but I feel as though I may have gotten into bad habits from what certain people in this thread are telling me. I think I can pretty much ace the rest of this course, this is only a minor setback.

[sohguanh]

Athar wrote: I guess it's obvious that your instructor has a mathematical background - his solution is closer to how you would express the sum in mathematical terms, so it's understandable he considers it to be the more "natural" solution.

I think Athar sums up how I feel. The TA code snippet (2) model more closely in mathematical terms. However if the TA comment on your code snippet (1) as 'poor programming style' I would say it is not. In fact, it is much clearer for your code snippet (1).

As for performance concerns, with modern CPU and C++ compiler, I doubt the run-time would matter a lot. One way to test is to run the for loop 1 million times perhaps before a noticeable performance can be felt.

My humble opinion.

for (i=0; i<TERMS; i++)
{
  divisor += 2.0;  // Increment divisor.
  add ? (total += 1/divisor) : (total -= 1/divisor);
  add = !add; // Flip mathematical operation for next loop increment.
}

[firedraco]

I got hammered on my first assignment for 'poor programming style'.

90% of the time, programming style is just opinion...but in this case...it's still an opinion. :P

The only thing that looks strange to me is the switch case, but I wouldn't call that "poor programming style." Especially in an INTRO to programming course. >_>