Explore our sites

Stack Exchange
tour help
Take the tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

My task is:

Write a program that reads integers until 0 is entered. After input terminates, the program should report the total number of even integers (excluding the 0) entered, the average value of the even integers, the total number of odd integers entered, and the average value of the odd integers.

But I had to use switch for this...

My code is:

#include <stdio.h>

int main(void)

{
    int oddIndex = 0, evenIndex = 0, evenTotal = 0, evenSum, oddTotal = 0, oddSum;
    int userInput;
    int oddArray[oddIndex], evenArray[evenIndex];

    printf("please enter some numbers: \n");
    while (((scanf("%d", &userInput)) == 1) && (userInput != 0))
    {

        switch (userInput%2)
        {
            case 0:
                evenArray[evenIndex] = userInput;
                evenIndex++;
                evenTotal++;
                evenSum += userInput;
                break;

            default:
                oddArray[oddIndex] = userInput;
                oddIndex++;
                oddTotal++;
                oddSum += userInput;
                break;
        };
    }
    if (evenTotal > 0)
    printf("even total: %d, even average: %d\n", evenTotal, evenSum/evenTotal);
    else
        printf("there is no even numbers!\n");
    if (oddTotal > 0)
    printf("odd total: %d, odd average: %d", oddTotal, oddSum/oddTotal);
    else
        printf("there is no odd numbers!");
}

Is this code ok?

share|improve this question
 
It's a little weird that you're using a switch for a 2-case scenario; an if-else would be more appropriate. –  seand Jan 28 at 23:48
 
You have to? Is this an homework? –  akappa Jan 28 at 23:49
 
yes i know, its kind of stupid. but in the task, first he said to do this with if else, but couple of exercises latter he asked to redo this exercise using switch..@seand –  Nir Jan 28 at 23:50
 
yes, i have to use switch for this task. its not homework, im reading the C primer plus book @akappa –  Nir Jan 28 at 23:50
add comment

2 Answers

up vote 4 down vote accepted

I haven't been a C guy for years, but a few observations:

You have int oddArray[oddIndex], evenArray[evenIndex]; declared, but the only time you use it is to add the entered number, and even that will fail because you have basically declared them as size[0], so oddArray[1] will throw an exception.

I think [even|odd]Index and [even|odd]Total can be combined into one variable.

I'm not sure what the standard is for C, but I really don't like the multiple declarations on one line. There should be a single declaration per line. Its less confusing that way.

You could also put some error handling around the input. What happens if somebody enters "A"? Your program would crash. I also think your instruction should be a little more clear. What does "please enter some numbers." really mean?

Minor thing, you forgot to indent the line after the if's at the bottom.

I would also move the reporting to a different function and reduce duplication.

Your code could turn out like this:

#include <stdio.h>

void DisplayResults(char numberType[], int total, int sum);

int main(void)    
{
    int oddCount= 0;
    int evenCount= 0;
    int evenSum = 0;
    int oddSum = 0;
    int userInput;

    printf("Please enter a number followed by <enter>.  Enter \"0\" to exit: \n");

    while (((scanf("%d", &userInput)) == 1) && (userInput != 0))
    {

        switch (userInput%2)
        {
            case 0:
                evenCount++;
                evenSum += userInput;
                break;

            default:
                oddCount++;
                oddSum += userInput;
                break;
        };
    }

    DisplayResults("Even", evenCount, evenSum);
    DisplayResults("Odd", oddCount, oddSum);
    return 0;
}

void DisplayResults(char numberType[], int count, int sum)
{
    if (total > 0)
    {
        printf("%s total: %d, %s average: %d\n", numberType, count, numberType, sum/count);
    }
    else
    {
        printf("There are no %s numbers!\n", numberType);
    }
}

Please excuse me if I have the syntax wrong, I haven't used C in about 20 years.

share|improve this answer
add comment
up vote 3 down vote

  1. It does not work well on my machine (gcc version 4.4.5):

    please enter some numbers: 
1
2
3
0
even total: 1, even average: 134519058
odd total: 2, odd average: 67257214

  2. The size of oddArray and evenArray is zero. (oddArray[oddIndex], where oddIndex is zero) (See also: How to initialize an array in C

  3. Actually, you could remove these arrays oddIndex, and evenIndex, they are only written, never read.

  4. There are missing \ns in the last two printfs.

  5. From Code Complete, 2nd Edition, p761:

    Use only one data declaration per line

    [...] It’s easier to modify declarations because each declaration is self-contained.

    [...]

    It’s easier to find specific variables because you can scan a single column rather than reading each line. It’s easier to find and fix syntax errors because the line number the compiler gives you has only one declaration on it.

  6. The names evenTotal and oddTotal are confusing since you have evenSum and oddSum too. I'd call them evenCount and oddCount.

share|improve this answer
 
Hi, i initialized the odd/even numbers array to zero before entering the loop to 0, so it will store the first number in place0, and then I increment this index by one so the next time i will store a number it will go to the next place in the array...how should i'v done it? im reading "C primer plus" and i saw this method over there, but maybe I got it wrong..thanks! @palacsint –  Nir Jan 29 at 1:52
 
@user1959174: I'm not too familiar with C, but I think you should use malloc here. Anyway, those arrays are unnecessary here, so I would simply remove them. –  palacsint Jan 29 at 11:37
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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