0
\$\begingroup\$

Any suggestions or comments? Is my style any good?

Initially I struggled with this problem as I found that using the "+=" operator gave the wrong results. I also tried a while loop the first time which was a baaaad idea. But what actually worked was using a static accumulator variable.

#include <stdio.h>





static unsigned long total;

int main(void)
{


  total = 0;





  for(unsigned int i = 1; i < 334; i++){

     total  = total + (3*i);


  }




  for(unsigned int j = 1; j < 1000; j++){

    if(j%5==0 && j%3!=0){

      total  = total + j;

    }else{

      total = total + 0;

    }


  }



  printf("Project Euler problem 1 solution = ");
  printf("%lu \n", total);


  return 0;
}

Ref: Euler 1: Multiples of 3 and 5

\$\endgroup\$

closed as off-topic by πάντα ῥεῖ, Stephen Rauch, Sᴀᴍ Onᴇᴌᴀ, t3chb0t, Donald.McLean Mar 29 '18 at 12:03

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – πάντα ῥεῖ, Stephen Rauch, Sᴀᴍ Onᴇᴌᴀ, t3chb0t, Donald.McLean
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 4
    \$\begingroup\$ Is the copious amount of whitespace intended? \$\endgroup\$ – yuri Mar 28 '18 at 18:43
  • \$\begingroup\$ Please explain what that code is intended to do, briefly in the question title, and in more depth in the question body. As is your question lacks enough context to make it useful for future research. \$\endgroup\$ – πάντα ῥεῖ Mar 29 '18 at 3:20
2
\$\begingroup\$

Your code is very hard to read since there is so much empty space. Even if we don't remove all of them, the following variant is a lot easier to read:

#include <stdio.h>

static unsigned long total;

int main(void)
{
  total = 0;

  for(unsigned int i = 1; i < 334; i++){
     total  = total + (3*i);
  }

  for(unsigned int j = 1; j < 1000; j++){

    if(j%5==0 && j%3!=0){
      total  = total + j;
    }else{
      total = total + 0;
    }
  }

  printf("Project Euler problem 1 solution = ");
  printf("%lu \n", total);

  return 0;
}

It fits even into the code example without any scrollbars.


There is no need to make total global. It should be a local variable.


Your first for loop uses an optimization, your second doesn't. That's asymmetrical. Use the same trick for both loops.


total = total + 0 doesn't change total at all. The compiler will most likely throw that line away, but better don't include it.


The following for loop is easier to understand:

for(unsigned int i = 0; i < 1000; i += 3) {
    total += i;
}

Concerning your comments on +=, did you accidentally use total += total + j? This will give wrong results. Otherwise I cannot reproduce your behaviour.


If you printf a unsigned int, you must use %u, not %lu, since the latter is meant for unsigned long int. Your compiler will warn you mismatching specifiers if you enable warnings.


If you apply those comments above, you end up with

#include <stdio.h>

int main(void)
{
  unsigned int total = 0;

  for(unsigned int i = 0; i < 1000; i += 3){
     total += i;
  }

  for(unsigned int i = 0; i < 1000; i += 5){    
    if(i % 3 != 0){
      total += i;
    }
  }

  printf("Project Euler problem 1 solution = %u \n", total);

  return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Mismatched specifier/type with unsigned int total = 0; .... printf("Project Euler problem 1 solution = %lu \n", total);. This implies code was not compiled with a well enabled compiler. \$\endgroup\$ – chux Mar 28 '18 at 23:16
  • \$\begingroup\$ @chux probably, but I have no idea what compiler OP uses. Added a section, thanks. \$\endgroup\$ – Zeta Mar 29 '18 at 5:43
1
\$\begingroup\$
  • Avoid unnecessary vertical spacing. On the other hand, a horizontal spacing around operators, braces, etc is necessary.

  • Prefer += where possible.

  • total = total + 0; is a no-op. Remove it, along with an entire else clause.

  • The j loop can be sped up 5-fold by incrementing j by five:

        for (int j = 0; j < 1000; j += 5) {
            if (j % 3 != 0) {
                total += j;
            }
        }
    

    Along the same line, the i loop can be rewritten as

        for (int i = 0; i < 1000; i += 3) {
            total += i;
        }
    

    Among other things, it avoids a magic number 334, which needs some explanation (if you feel compelled to use your form, make it a constant with the meaningful name).

  • That said, ProjectEuler problems are more about math than programming. Problem 1 doesn't need any coding whatsoever.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.