2
\$\begingroup\$

I have the following assignment:

A program that takes four (4) inputs then using a conditional statement does the following:

  • Display the values that are below or equal to 25.
  • Multiply the values that are below 20 with each other.
  • Add the values that are above 25 with each other.

I've successfully solved all of them as they're pretty easy, however I want to ask a couple of things along with getting feedback from you.

  1. I know that using less variables is always a better choice if you can do it, it might not matter on such small scale projects but I believe it's useful to get in the habit head-on. So in this specific example code, can I use a single var instead of 4 different ones? I know I can for the second and third tasks, however what about the third? Can I still list the inputs that are less than or equal to 25?

  2. What tips do you have to make the code readable and even more easy to understand?

Also keep in mind that using loops, arrays, functions ... is not allowed so I would be glad if we keep our discussion limited to conditionals only.

#include "stdafx.h"
#include "iostream"
using namespace std;

int _tmain(int argc, _TCHAR* argv[])
{
    float val1, val2, val3, val4;
    float sum = 0, multiply = 1;

    cout << "Enter the first number: ";
    cin >> val1;
    if(val1 < 20) {
        multiply *= val1;
    } else if(val1 > 25) {
        sum += val1;
    }

    cout << "Enter the second number: ";
    cin >> val2;
    if(val2 < 20) {
        multiply *= val2;
    } else if(val2 > 25) {
        sum += val2;
    }

    cout << "Enter the third number: ";
    cin >> val3;
    if(val3 < 20) {
        multiply *= val3;
    } else if(val1 > 25) {
        sum += val3;
    }

    cout << "Enter the fourth number: ";
    cin >> val4;
    if(val4 < 20) {
        multiply *= val4;
    } else if(val4 > 25) {
        sum += val4;
    }

    if(val1 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val1 << endl;
    if(val2 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val2 << endl;
    if(val3 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val3 << endl;
    if(val4 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val4 << endl;

    cout << "When we multiply the numbers below 20 we get: " << multiply << endl;
    cout << "When we add the numbers above 25 we get: " << sum << endl;

    return 0;
}
\$\endgroup\$
2
\$\begingroup\$

Regarding the number of variables:

For displaying all "small" values, you could introduce a variable of type string in which you save all small numbers. But since you haven't yet learned about functions and arrays, this would make your code much more complicated than it is now.

So for now, just keep the program as-is, using one variable per number.

In the rest of the review, I'm going straight from the top to the bottom of your code.

#include "stdafx.h"
#include "iostream"

The #include "stdafx.h" is fine. When an include file comes from your own project (which stdafx.h does), you should surround it with double quotes.

But all other include files (especially system headers like iostream) should not be enclosed in "double quotes" but in <angle brackets>. So it should be #include <iostream>.

using namespace std;

As soon as you start writing serious code, you should omit the using namespace std. This line imports several hundred names from the std namespace, such as min, max, sort, depending on the system headers you included. This can lead to confusion when you make typos or just forget to declare a function that is also defined in that namespace.

int _tmain(int argc, _TCHAR* argv[])
{
    float val1, val2, val3, val4;
    float sum = 0, multiply = 1;

Whenever possible, use the data type double instead of float, since it is more precise (15 digits instead of 7).

    cout << "Enter the first number: ";
    cin >> val1;

Imagine what happens when the user of the program doesn't enter a number here but simply types hello program. how are you today?. In that case val1 will not get any value, and whatever value was stored there before will be used by the program. This leads to unpredictable behavior.

To fix this, write the following code:

    if (!(std::cin >> val1)) {
        std::cerr << "Error: could not read the first number\n";
        return 1; // Any nonzero value means failure.
    }

Continuing with your code …

    if(val1 < 20) {
        multiply *= val1;
    } else if(val1 > 25) {
        sum += val1;
    }

Very nice. Short and readable. I would just change the variable name multiply into product, since variables usually get passive names. Later, when you write functions, these will get the active names, like multiply two numbers.

    cout << "Enter the second number: ";
    cin >> val2;
    if(val2 < 20) {
        multiply *= val2;
    } else if(val2 > 25) {
        sum += val2;
    }

    cout << "Enter the third number: ";
    cin >> val3;
    if(val3 < 20) {
        multiply *= val3;
    } else if(val1 > 25) {
        sum += val3;
    }

    cout << "Enter the fourth number: ";
    cin >> val4;
    if(val4 < 20) {
        multiply *= val4;
    } else if(val4 > 25) {
        sum += val4;
    }

    if(val1 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val1 << endl;
    if(val2 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val2 << endl;
    if(val3 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val3 << endl;
    if(val4 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val4 << endl;

The are/is doesn't look convincing. To make the output nice, you could count the numbers that are less than or equal to 25. Then, add the following code:

    if (lessOrEqual25Count == 0) {
        std::cout << "No numbers are below or equal to 25.\n";

    } else {
        if (lessOrEqual25Count == 1) {
            std::cout << "One number is below or equal to 25:";
        } else {
            std::cout << lessOrEqual25Count << " numbers are below or equal to 25:";
        }
        if (val1 <= 25) std::cout << " " << val1;
        if (val2 <= 25) std::cout << " " << val2;
        if (val3 <= 25) std::cout << " " << val3;
        if (val4 <= 25) std::cout << " " << val4;
        std::cout << "\n";
    }

Continuing with your code …

    cout << "When we multiply the numbers below 20 we get: " << multiply << endl;
    cout << "When we add the numbers above 25 we get: " << sum << endl;

    return 0;
}

All in all, this is a nice program. It works fine, and it's good that you will learn about arrays and functions soon. They will really make programming easier, since currently you have to repeat large sections of your code over and over.

\$\endgroup\$
  • \$\begingroup\$ As an addition I would put the duplicated code such as if(val2 < 20) { multiply *= val2; } else if(val2 > 25) { sum += val2; } Into a new function and pass parameters, so the input for example and return the new sum value. sum = decideMath(currentSum, input3) \$\endgroup\$ – QVDev Dec 14 '16 at 23:16
  • \$\begingroup\$ The question explicitly states: no functions, since this is a very beginner's question. \$\endgroup\$ – Roland Illig Dec 14 '16 at 23:40
  • \$\begingroup\$ This is a great answer! Wish I can get such feedback along the way! Thanks a lot man! \$\endgroup\$ – Saad Sawash Dec 15 '16 at 6:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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