Pretty proud of this program i wrote, any critiques?

**Rednally** · 03-22-2016

Anything a more advanced programmer would say I should be looking to do for a program like this?
Any tips or tricks?

Thanks!

Edit for clarity: My code works 100% as intended, just wondering if there were smarter ways to go about doing it!

Code:

#include <stdio.h>#include <stdlib.h>
/*
Created by Jake Yllander
3/20/2016
COP 1000 Intro Programming Concepts
Assignment 8 - Problem Solving with switch


Store the 5 integers from the user. Determine the largest number, the smallest number,
the sum, and the average of the 5 integers. Create a choice menu to display one 
of the four options you just determined. Receive the choice input from the user 
and display the answer to one of the four options in the choice menu. The code should 
also tell the user when they have input an invalid choice.


*/
main() {
    int number=0, numberCount = 0,smallest=0,largest=0,sum=0,choice=0;
    double average=0;
    while (numberCount != 5) {
        if (number > largest) {
            largest = number;
        }
        else{
            if (number < smallest) {
                smallest = number;
            }
        }


        printf("Enter a number:\n");
        scanf("%i", &number);
        smallest = number;
        numberCount++;
        sum += number;
        average = (double)sum / numberCount;
    }
    do {
        printf("Choose one of the following optons\n");
        printf("1. Display the smallest number entered\n");
        printf("2. Display the largest number entered\n");
        printf("3. Display the sum of the numbers entered\n");
        printf("4. Display the average of the numbers entered\n");
        printf("5. Exit\n");
        scanf("%i", &choice);


        switch (choice) {
        case 1:
            printf("Smallest number is:%i\n", smallest);
            break;
        case 2:
            printf("Largest number is:%i\n", largest);
            break;
        case 3:
            printf("The sum of the numbers is:%i\n", sum);
            break;
        case 4:
            printf("The average of the numbers is:%.2lf\n", average);
            break;
        case 5:
            break;
        default:
            printf("You entered an invalid choice!\n");
            break;
        }
    } while (choice != 5);
    system("pause");
}

**Matticus** · 03-22-2016

Hmm.

Code:

Enter a number:
1
Enter a number:
2
Enter a number:
3
Enter a number:
4
Enter a number:
5
Choose one of the following optons
1. Display the smallest number entered
2. Display the largest number entered
3. Display the sum of the numbers entered
4. Display the average of the numbers entered
5. Exit
1
Smallest number is:5
Choose one of the following optons
1. Display the smallest number entered
2. Display the largest number entered
3. Display the sum of the numbers entered
4. Display the average of the numbers entered
5. Exit
2
Largest number is:4
Choose one of the following optons
1. Display the smallest number entered
2. Display the largest number entered
3. Display the sum of the numbers entered
4. Display the average of the numbers entered
5. Exit
5
Press any key to continue . . .

**laserlight** · 03-22-2016

The forum software probably messed up the formatting for this:

Code:

#include <stdio.h>#include <stdlib.h>

So I presume it was:

Code:

#include <stdio.h>
#include <stdlib.h>

My comments:

main() should be int main(void)
It would be good to use a named constant instead of the magic number 5 in numberCount != 5.
You seem to be checking the value of number before the user entered the number.
By initialising smallest=0, you assume that the smallest number entered will be non-positive. If the smallest number entered is 1, your algorithm will produce an incorrect result. You could #include <limits.h> and initialise smallest to INT_MAX instead. Alternatively, set the initial value of smallest to the first number entered.
By initialising largest=0, you assume that the largest number entered will be non-negative. If the largest number entered is -1, your algorithm will produce an incorrect result. You could #include <limits.h> and initialise largest to INT_MIN instead. Alternatively, set the initial value of largest to the first number entered.
Remember to check the return value of scanf. If there is an input error, perhaps you could handle and/or report it in some way.
You set smallest = number on every iteration of the loop that reads input. Surely this is a mistake.
The average probably should be computed after all the input is done rather than overwritten on every iteration.
Note that system("pause") makes your code unnecessarily non-portable. I suggest either running your program from a separate command prompt window, or setting your IDE to pause the program for you.

Remember, just getting the program to compile is an early step. You need to actually test your program and fix bugs to make sure that it works before you start being proud of it.

EDIT:

Originally Posted by Rednally

Edit for clarity: My code works 100% as intended, just wondering if there were smarter ways to go about doing it!

What were your test input? Matticus found bugs. I found bugs. Your 100% claim is utterly bogus, unless you are saying that you don't care about the requirements, in which case there are no smarter ways of doing this: you have written the model reference implementation by definition, any other implementations are inferior because they are different.

**algorism** · 03-22-2016

A couple of points not yet mentioned:

Do you really want to read octal and hex values as well as decimal? If not, then use %d instead of %i in scanf. Using %i causes an input like 077 to equate to decimal 63 (7 * 8 + 7). I also encourage people to use %d in printf (even though there's no difference between %d and %i in printf) just to remind you that it's the "normal" one.

In the printf, don't use %lf. Although that's necessary in scanf to read a double, it's incorrect in printf (for reasons I won't go into here). For printf, you use %f for both float and double, and %Lf for long double (note the capital L).

**Rednally** · 03-22-2016

Originally Posted by Matticus

Hmm.

Code:

Enter a number:
1
Enter a number:
2
Enter a number:
3
Enter a number:
4
Enter a number:
5
Choose one of the following optons
1. Display the smallest number entered
2. Display the largest number entered
3. Display the sum of the numbers entered
4. Display the average of the numbers entered
5. Exit
1
Smallest number is:5
Choose one of the following optons
1. Display the smallest number entered
2. Display the largest number entered
3. Display the sum of the numbers entered
4. Display the average of the numbers entered
5. Exit
2
Largest number is:4
Choose one of the following optons
1. Display the smallest number entered
2. Display the largest number entered
3. Display the sum of the numbers entered
4. Display the average of the numbers entered
5. Exit
5
Press any key to continue . . .

Wow thats weird, I have no idea why my programs acting like that.
Why would it do that for numbers (1,2,3,4,5) but not something like (18, 21, 17, 44, 9)?
Pretty proud of this program i wrote, any critiques?-hmm-png

Pretty proud of this program i wrote, any critiques?-hmm-png

Originally Posted by laserlight

The forum software probably messed up the formatting for this:

Code:

#include <stdio.h>#include <stdlib.h>

So I presume it was:

Code:

#include <stdio.h>
#include <stdlib.h>

My comments:

main() should be int main(void)
It would be good to use a named constant instead of the magic number 5 in numberCount != 5.
You seem to be checking the value of number before the user entered the number.
By initialising smallest=0, you assume that the smallest number entered will be non-positive. If the smallest number entered is 1, your algorithm will produce an incorrect result. You could #include <limits.h> and initialise smallest to INT_MAX instead. Alternatively, set the initial value of smallest to the first number entered.
By initialising largest=0, you assume that the largest number entered will be non-negative. If the largest number entered is -1, your algorithm will produce an incorrect result. You could #include <limits.h> and initialise largest to INT_MIN instead. Alternatively, set the initial value of largest to the first number entered.
Remember to check the return value of scanf. If there is an input error, perhaps you could handle and/or report it in some way.
You set smallest = number on every iteration of the loop that reads input. Surely this is a mistake.
The average probably should be computed after all the input is done rather than overwritten on every iteration.
Note that system("pause") makes your code unnecessarily non-portable. I suggest either running your program from a separate command prompt window, or setting your IDE to pause the program for you.

Remember, just getting the program to compile is an early step. You need to actually test your program and fix bugs to make sure that it works before you start being proud of it.

EDIT:

What were your test input? Matticus found bugs. I found bugs. Your 100% claim is utterly bogus, unless you are saying that you don't care about the requirements, in which case there are no smarter ways of doing this: you have written the model reference implementation by definition, any other implementations are inferior because they are different.

Laserlight thanks for the comments, but youre losing me with the jargon because I am not yet at that level to understand what youre saying.

- Its not correct to check the value of a number before the user has inputted it right?
- We just learned constants actually so I know how to do this!
- "

By initialising smallest=0, you assume that the smallest number entered will be non-positive. If the smallest number entered is 1, your algorithm will produce an incorrect result. You could #include <limits.h> and initialise smallest to INT_MAX instead. Alternatively, set the initial value of smallest to the first number entered.
By initialising largest=0, you assume that the largest number entered will be non-negative. If the largest number entered is -1, your algorithm will produce an incorrect result. You could #include <limits.h> and initialise largest to INT_MIN instead. Alternatively, set the initial value of largest to the first number entered."

I haven't learned what #include <limits.h> does. Im trying to understand what youre saying here, I have set all my variables = 0, so from what I can get from this is that because smallest = 0 when I declared my int, my code will be wrong when smallest = 1? (This would help with what matticus pointed out also)
I have never heard of <limits.h> or INT_MAX or INT_MIN are those meant to be constants?
Lastly, you've told me to do this a lot in the posts you helped me in (set initial value of largest to the first number entered) but I have no clue how to do that!

I guess thats what I was trying to do when I wrote smallest=number; outside of the loop so that the first number entered got the value of smallest, and every value after that would go through the loop to determine if it was smaller or larger than the first number entered.

So far my professor wants us to use main() and system ("pause") because we have't been in to that much detail about what these even do!

When I said 100% as intended (well now I know im utterly stupid for saying that) I meant for the test case which I should have provided! (18, 21, 17, 44, 9)

Anyways, thanks guys!

**Rednally** · 03-22-2016

Originally Posted by algorism

A couple of points not yet mentioned:

Do you really want to read octal and hex values as well as decimal? If not, then use %d instead of %i in scanf. Using %i causes an input like 077 to equate to decimal 63 (7 * 8 + 7). I also encourage people to use %d in printf (even though there's no difference between %d and %i in printf) just to remind you that it's the "normal" one.

In the printf, don't use %lf. Although that's necessary in scanf to read a double, it's incorrect in printf (for reasons I won't go into here). For printf, you use %f for both float and double, and %Lf for long double (note the capital L).

In the videos that my professor has online, he always uses %lf in the printf

Could this be because the only data variables we know so far are double/int ?

**algorism** · 03-22-2016

Originally Posted by Rednally

In the videos that my professor has online, he always uses %lf in the printf

Could this be because the only data variables we know so far are double/int ?

The C99 standard says that adding l (lowercase ell) before f in a format spec has no effect. See the bottom of page 276 and the top of 277 in the draft:
http://www.open-std.org/JTC1/SC22/WG...docs/n1256.pdf
(actually, you probably shouldn't read it but maybe your prof should)

It could be that he's using it so that you remember to use it in scanf, where it actually is necessary.

**whiteflags** · 03-22-2016

Originally Posted by Rednally

Why would it do that for numbers (1,2,3,4,5) but not something like (18, 21, 17, 44, 9)?

Because your program assumes that the last number entered is the smallest in the series, and that could be true sometimes. Look carefully again at the code.

Code:

    while (numberCount != 5) {
        if (number > largest) {
            largest = number;
        }
        else{
            if (number < smallest) {
                smallest = number;
            }
        }
 
 
        printf("Enter a number:\n");
        scanf("%i", &number);
        smallest = number;
        ...
}

See the problem? The last number entered is not checked to be the smallest at all, it is assigned.

**jimblumberg** · 03-23-2016

In the printf, don't use %lf. Although that's necessary in scanf to read a double, it's incorrect in printf (for reasons I won't go into here). For printf, you use %f for both float and double, and %Lf for long double (note the capital L).

The C99 standard says that adding l (lowercase ell) before f in a format spec has no effect.

Which means that from C99 onward using a printf() format specifier of "%lf" for a double is acceptable, since the compiler will ignore that "extra" character.

Jim

**Rednally** · 03-23-2016

Originally Posted by whiteflags

Because your program assumes that the last number entered is the smallest in the series, and that could be true sometimes. Look carefully again at the code.

Code:

    while (numberCount != 5) {
        if (number > largest) {
            largest = number;
        }
        else{
            if (number < smallest) {
                smallest = number;
            }
        }
 
 
        printf("Enter a number:\n");
        scanf("%i", &number);
        smallest = number;
        ...
}

See the problem? The last number entered is not checked to be the smallest at all, it is assigned.

So the problem with this program, is the statement smallest = number; OUTSIDE of the loop, not the statement inside of the loop?

I have no clue how to set the first value to smallest if thats not how you do it! Any tips then?

**Matticus** · 03-23-2016

Originally Posted by Rednally

So the problem with this program, is the statement smallest = number; OUTSIDE of the loop, not the statement inside of the loop?

No, the problem is you assign the input to "smallest" without doing any checking whether it actually is the smallest value.

Originally Posted by Rednally

I have no clue how to set the first value to smallest if thats not how you do it! Any tips then?

As I've said in two of your other threads, the first time around, set the largest and smallest to the input.

**whiteflags** · 03-23-2016

Originally Posted by Rednally

So the problem with this program, is the statement smallest = number; OUTSIDE of the loop, not the statement inside of the loop?

Unless I'm blind, the statement that I am talking about is inside the loop.

I have no clue how to set the first value to smallest if thats not how you do it! Any tips then?

Set smallest to, say INT_MAX to start, get the number, and check if the number is the new smallest, in that order. Similarly for largest.

**Rednally** · 03-23-2016

Originally Posted by Matticus

No, the problem is you assign the input to "smallest" without doing any checking whether it actually is the smallest value.

As I've said in two of your other threads, the first time around, set the largest and smallest to the input.

I dont know how to do that. How can I set the input to both the smallest AND the largest? What do you mean by only the first time around?

We haven't learned INT_MAX yet so there has to be a way to do it without INT_MAX that I am missing.

**Matticus** · 03-23-2016

Originally Posted by Rednally

I dont know how to do that. How can I set the input to both the smallest AND the largest?

You have that the wrong way around. Set the smallest and largest to the value of the first input.

Code:

smallest = largest = number;  // This sets "smallest" to the value of "number",
                              // as well as setting "largest" to the value of "number".

Originally Posted by Rednally

What do you mean by only the first time around?

Okay, let me try to break down the thought process here.

You have three variables - "smallest" which holds the smallest number entered, "largest" which holds the largest number entered, and "number" which represents the user input.

Initially, "smallest" and "largest" have no meaningful values. Nothing has been entered yet, so there are no smallest or largest values yet.

Then the user enters their very first number. Since there is only one number entered by the user at this point, you set both "smallest" and "largest" to this value. For instance, if the user enters 5, then 5 is both the smallest and largest number entered by the user at that point.

Now the user enters a second number. If this value is less than the first number, then we need to update the value of "smallest". If this value is greater than the first number, then we need to update the value of "largest". Otherwise, we don't need to update either one.

The user then enters a third number, and the process repeats.

By setting "smallest" and "largest" to the very first number when it is first entered, we have a valid basis of comparison for each subsequent number entered by the user.

**Rednally** · 03-24-2016

Originally Posted by Matticus

You have that the wrong way around. Set the smallest and largest to the value of the first input.

Code:

smallest = largest = number;  // This sets "smallest" to the value of "number",
                              // as well as setting "largest" to the value of "number".

Okay, let me try to break down the thought process here.

You have three variables - "smallest" which holds the smallest number entered, "largest" which holds the largest number entered, and "number" which represents the user input.

Initially, "smallest" and "largest" have no meaningful values. Nothing has been entered yet, so there are no smallest or largest values yet.

Then the user enters their very first number. Since there is only one number entered by the user at this point, you set both "smallest" and "largest" to this value. For instance, if the user enters 5, then 5 is both the smallest and largest number entered by the user at that point.

Now the user enters a second number. If this value is less than the first number, then we need to update the value of "smallest". If this value is greater than the first number, then we need to update the value of "largest". Otherwise, we don't need to update either one.

The user then enters a third number, and the process repeats.

By setting "smallest" and "largest" to the very first number when it is first entered, we have a valid basis of comparison for each subsequent number entered by the user.

Oh my god, finally it clicks. I think the problem was that you thought smallest = largest = number; was something I was smart enough to figure out haha! Just kidding, but really thank you matticus I had no idea I could even do that in C.

Thread: Pretty proud of this program i wrote, any critiques?

Thread Tools

Search Thread

Display

Pretty proud of this program i wrote, any critiques?

Similar Threads

Converting a program I wrote in c++ to c

A little program I wrote for fun !

Just wrote my first Curses program

filesplitting program I wrote

Can someone look over the program I wrote?