views:

674

answers:

11
+2  Q: 

simple C problem

Hi, I have had to start to learning C as part of a project that I am doing. I have started doing the 'euler' problems in it and am having trouble with the first one. I have to find the sum of all multiples of 3 or 5 below 1000. Could someone please help me. Thanks.

#include<stdio.h>
int start;
int sum;

int main() {
    while (start < 1001) {
        if (start % 3 == 0) {
            sum = sum + start;
            start += 1;
        } else {
            start += 1;
        }

        if (start % 5 == 0) {
            sum = sum + start;
            start += 1;
        } else {
            start += 1;
        }
        printf("%d\n", sum);
    }
    return(0);
}
+2  A: 

You haven't said what the program is supposed to do, or what your problem is. That makes it hard to offer help.

At a guess, you really ought to initialize start and sum to zero, and perhaps the printf should be outside the loop.

KeyserSoze
In C, global variables are automatically initialized to 0, so that part technically isn't incorrect. However, I feel that it's bad practice to rely on that, and that one should explicitly initialize globals to 0 anyways.
Adam Rosenfield
+6  A: 

You would be much better served by a for loop, and combining your conditionals.

Not tested:

int main()
{
  int x;
  int sum = 0;

  for (x = 1; x <= 1000; x++)
    if (x % 3 == 0 || x % 5 == 0)
      sum += x;

  printf("%d\n", sum);
  return 0;
}
anthony
"You are erroneously incrementing start twice for certain iterations of your while loop" good catch.. I didn't see it...
tekBlues
oh, actually, if the counter is being updated in all 4 (eek) blocks, every number will be checked. it's somewhat cumbersome though.
anthony
Every number will be checked, but it won't be checked for both 3-divisibility and 5-divisibility. Each number will only be checked for one or the other.
Steve Jessop
+17  A: 

You could change your ifs:

 if  ((start % 3 == 0) || (start % 5 == 0)) 
     sum += start;
 start ++;

and don´t forget to initialize your sum with zero and start with one. Also, change the while condition to < 1000.

fbinder
You beat me to it!, the question cannot be worst documented...
tekBlues
sum += start surely?
Chris Simpson
And use local variables, not globals. And consider the merits of a for loop when you are doing a simple count: for (start = 3; start < 1000; start++) { ... }
Jonathan Leffler
+3  A: 

Eh right, well i can see roughly where you are going, I'm thinking the only thing wrong with it has been previously mentioned. I did this problem before on there, obviously you need to step through every multiple of 3 and 5 and sum them. I did it this way and it does work:

int accumulator = 0;
int i;

for (i = 0; i < 1000; i += 3)
 accumulator += i;

for (i = 0; i < 1000; i +=5) {
 if (!(i%3==0)) {
  accumulator += i;
 }
}
printf("%d", accumulator);

EDIT: Also note its not 0 to 1000 inclusive, < 1000 stops at 999 since it is the last number below 1000, you have countered that by < 1001 which means you go all the way to 1000 which is a multiple of 5 meaning your answer will be 1000 higher than it should be.

Toby
+2  A: 

Really you need a debugger, and to single-step through the code so that you can see what it's actually doing. Your basic problem is that the flow of control isn't going where you think it is, and rather than provide correct code as others have done, I'll try to explain what your code does. Here's what happens, step-by-step (I've numbered the lines):

1:    while (start < 1001) {
2:        if  (start % 3 == 0) {
3:            sum = sum + start;
4:            start += 1;
5:        }
6:        else {
7:            start += 1;
8:        }
9:
10:       if (start % 5 == 0) {
11:           sum = sum + start;
12:           start += 1;
13:       }
14:       else {
15:           start += 1;
16:       }
17:       printf("%d\n", sum);
18:    }
  • line 1. sum is 0, start is 0. Loop condition true.
  • line 2. sum is 0, start is 0. If condition true.
  • line 3. sum is 0, start is 0. sum <- 0.
  • line 4. sum is 0, start is 0. start <- 1.
  • line 5. sum is 0, start is 1. jump over "else" clause
  • line 10. sum is 0, start is 1. If condition false, jump into "else" clause.
  • line 15. sum is 0, start is 1. start <- 2.
  • line 16 (skipped)
  • line 17. sum is 0, start is 2. Print "0\n".
  • line 18. sum is 0, start is 2. Jump to the top of the loop.
  • line 1. sum is 0, start is 2. Loop condition true.
  • line 2. sum is 0, start is 2. If condtion false, jump into "else" clause.
  • line 7. sum is 0, start is 2. start <- 3.
  • line 10. sum is 0, start is 3. If condition false, jump into "else" clause.
  • line 15. sum is 0, start is 3. start <- 4.
  • line 17. sum is 0, start is 4. Print "0\n".

You see how this is going? You seem to think that at line 4, after doing sum += 1, control goes back to the top of the loop. It doesn't, it goes to the next thing after the "if/else" construct.

Steve Jessop
Um, where exactly is start set to 0, or sum set to 0?? You're wrong from line 1.
abelenky
ISO/IEC 9899:TC2 6.7.8:10 "If an object that has static storage duration is not initialized explicitly then ... if it has arithmetic type, it is initialized to (positive or unsigned) zero". The language in C89 is different but the result is the same: "If an object that has static storage duration is not initializedexplicitly, it is initialized implicitly as if every member that hasarithmetic type were assigned 0" (quote from a draft - I don't have a copy of the C89 standard). Both start and sum are set to 0 before `main()` is called.
Steve Jessop
+36  A: 
rampion
nice one, but there's a small inaccuracy: integer devision round sdown, not up; but as you wanted the floor funtion anyway (using ceil is wrong), the program should work as expected
Christoph
Doh! That's what I get for staying up late. Fixed, thanks!
rampion
upvoted for printf("186568\n"); as a perfectly valid solution ;)
Daren Thomas
Im sure if this was c++ someone would have come up with some template based voodoo that will do the whole thing at compile time....
Visage
Ah, if only. When will I ever learn? 990/2 = 495, not 445. Oh well. Fixed now.
rampion
@rampion: maybe you should use a program to check the calculation (of 990/2) for you :D
Jonathan Leffler
Cool. Are they supposed to be little empty boxes?
Lucas Jones
Brilliant answer. Learned lots of new things, if I cud i'd could id upvote my days votes away. Last program made me lol as well
xenon
+1 - Brilliant use of math. If there are ever any SO awards (other than badges, that is) handed out, this should be nominated for best answer. "And this year's 'Overflowed Cup' goes to.... rampion!"
Erich Mirabal
+1  A: 

The problem with your code is that your incrementing the 'start' variable twice. This is due to having two if..else statements. What you need is an if..else if..else statement as so:

           if  (start % 3 == 0) {
                    sum = sum + start;
                    start += 1;
            }
            else if (start % 5 == 0) {
                    sum = sum + start;
                    start += 1;
            }
            else {
                    start += 1;
            }

Or you could be more concise and write it as follows:

if(start % 3 == 0)
    sum += start;
else if(start % 5 == 0)
    sum += start;
start++;

Either of those two ways should work for you.

Good luck!

samoz
+3  A: 

The answers are all good, but won't help you learn C.

What you really need to understand is how to find your own errors. A debugger could help you, and the most powerful debugger in C is called "printf". You want to know what your program is doing, and your program is not a "black box".

Your program already prints the sum, it's probably wrong, and you want to know why. For example:

printf("sum:%d start:%d\n", sum, start);

instead of

printf("%d\n", sum);

and save it into a text file, then try to understand what's going wrong.

  • does the count start with 1 and end with 999?
  • does it really go from 1 to 999 without skipping numbers?
  • does it work on a smaller range?
G B
A: 

Here's a general solution which works with an arbitrary number of factors:

#include <stdio.h>

#define sum_multiples(BOUND, ...) \
    _sum_multiples(BOUND, (unsigned []){ __VA_ARGS__, 0 })

static inline unsigned sum_single(unsigned bound, unsigned base)
{
    unsigned n = bound / base;
    return base * (n * (n + 1)) / 2;
}

unsigned _sum_multiples(unsigned bound, unsigned bases[])
{
    unsigned sum = 0;

    for(unsigned i = 0; bases[i]; ++i)
    {
     sum += sum_single(bound, bases[i]);

     for(unsigned j = i + 1; bases[j]; ++j)
      sum -= sum_single(bound, bases[i] * bases[j]);
    }

    return sum;
}

int main(void)
{
    printf("%u\n", sum_multiples(999, 3, 5));
    return 0;
}
Christoph
+2  A: 

You have forgotten to initialize your variables,

Christy John
You're apparently the only one in all these answers that caught this little problem. Kudos!
abelenky
Thanks. I was also wondering why no one yet found out?
Christy John
Because it's not necessary (although it is advisable) to explicitly initialize an int variable with static storage duration. They're 0-inited for you.
Steve Jessop
I'm sorry I was ignorant of the fact. Thanks onebyone for the info,
Christy John
A: 

Starting new here, I see plenty of good advice above. Without reading it all (something I should do, but I want to see how the site works) I note your algorithm is broken.

You want to sum multiples of 3 and 5, but increment your way through all the possible integers without regard to the possibility a number could satisfy both. Such mysterious numbers as 15, 30, etc...

So, perform both modulus tests before presuming to increment to the next int?

mrk