3
\$\begingroup\$

Code works exactly how I want it to, but any simplification to the code that could be made would be much appreciated (granted this program uses almost everything within the scope of my C++ knowledge).

 // sum calculator
#include<iostream>
#include<string>

//integer sum: 0 + 1 + 2 + ... + n
int integer_sum(int a, int b){
    int int_sum(0);
    while(a <= b){
    int_sum += a;
    a++;
   }
   return int_sum;
}

//squares sum: 0^2 + 1^2 + 2^2 + ... + n^2
int square_sum(int a, int b){
    int squ_sum(0);
    while(a <= b){
    squ_sum += a*a;
    a++;
  }
  return squ_sum;
}

//cubes sum: 0^3 + 1^3 + 2^3 + ... + n^3
int cube_sum(int a, int b){
    int cub_sum(0);
    while(a <= b){
    cub_sum += a*a*a;
    a++;
   }
  return cub_sum;
}

int main(){

    while(std::cin){

        char choice;
        // user menu
        std::cout << "select an operation: \n"
                  << "(a) sum of integers\n"
                  << "(b) sum of squares\n"
                  << "(c) sum of cubes\n"
                  << "(d) calculator OFF\n";
        std::cin >> choice;

        std::string restraint("First value must be less than the second");
        std::string request_range_1("First Range value: ");
        std::string request_range_2("Second Range value: ");
        int range_1;
        int range_2;

        switch(choice){
        case 'd':
            break;
        case 'a':
            std::cout << restraint << std::endl;
            std::cout << request_range_1;
            std::cin >> range_1;

            std::cout << request_range_2;
            std::cin >> range_2;

            std::cout << std::endl << "The sum of the integers in the range: " << "[" << range_1 << "," << range_2 << "]\nis: ";
            std::cout << integer_sum(range_1, range_2) << std::endl << std::endl;
            break;
        case 'b':
            std::cout << restraint << std::endl;
            std::cout << request_range_1;
            std::cin >> range_1;

            std::cout << request_range_2;
            std::cin >> range_2;

            std::cout << std::endl << "The sum of the squares in the range: " << "[" << range_1 << "," << range_2 << "]\nis: ";
            std::cout << square_sum(range_1, range_2) << std::endl << std::endl;
            break;
        case 'c':
            std::cout << restraint << std::endl;
            std::cout << request_range_1;
            std::cin >> range_1;

            std::cout << request_range_2;
            std::cin >> range_2;

            std::cout << std::endl << "The sum of the cubes in the range: " << "[" << range_1 << "," << range_2 << "]\nis: ";
            std::cout << cube_sum(range_1, range_2) << std::endl << std::endl;
            break;
        default:
            std::cout << "Incorrect operation \n\n";
            break;
        }
        if(choice == 'd')
            break;
    }
    std::cout << "Calculator OFF" << std::endl;
}
\$\endgroup\$
0

3 Answers 3

2
\$\begingroup\$

Refactor IO

In each case of your switch statement, you have this at the very beginning:

std::cout << restraint << std::endl;
std::cout << request_range_1;
std::cin >> range_1;

std::cout << request_range_2;
std::cin >> range_2;

Rather than re-writing this out for each case, you should refactor this to its own function that returns the two inputs. That way, you can just call that function to get the inputs.

Also, speaking of inputs...


Never trust the user!

When you are grabbing the two input numbers for an operation, you show the user a restraint message that says that the first number must be smaller than the second. While your function technically do "handle" this by returning 0, it would be good practice to not call those functions at all and to instead print out to the user if they accidentally (or purposely) entered a bigger number first.

You could include this in the extracted input-getting function described above. You can even create a loop inside this that repeatedly asks the user for proper input before actually returning the input.


early exit -- early exit -- early exit -- early exit....

    if(choice == 'd')
        break;
}
std::cout << "Calculator OFF" << std::endl;

This is a bit of code I grabbed from the end of the switch statement. Note that the only time that this bit of code is going to be reached is if the user hit d as their input.

Therefore, you can do some simplifying here. Rather than exiting from the switch statement only to do another conditional on whether or not to break the loop, just return from the entire program as soon as it is discovered that the user entered 'd':

case 'd':
    std::cout << "Calculator OFF" << std::endl;
    return 0;

This is much simpler and removes the redundancy from your code.


More refactoring and simplification

Your three _sum functions are all very similar except for a single line. These lines:

int_sum += a;

and

squ_sum += a*a;

and

cub_sum += a*a*a;

The rest of the code in the function is identical (of course, except for the variable names). This means that we can do some refactoring.

To simplify the code, let's write a blank function that follows the same format as the others:

int sum_exponent_series(int a, int b, int exp);

Notice the last parameter: this will allow the function to know whether to put a, a*a, or a*a*a... Which, in fact, can be simplified using <cmath>'s pow.

Now, the body of the function looks like this:

int sum(0);
for(; a <= b; a++) {
    sum += pow(a, exp);
}
return sum;

Note, I made two other changes:

  • I fixed your indentation (watch that in the future)
  • I turned your while loop into a for loop. An incrementation at the end of a while loop like you is a pretty good indication you need a for loop.

Now, instead of having three functions, you can just have this one function and call it with different exp values.

BUT WAIT!! You can make your program even more flexible now! Rather than having the user enter the "type" of exponent series thing they want, have them input these three things:

  • A number
  • The amount of times to add that number
  • The exponent to raise the number to when adding

That way, you won't even need a switch statement at all! The user could enter numbers like 3, or 8, or anything else without having to be restricted by what options you already set in place!

\$\endgroup\$
2
\$\begingroup\$

@SirPython has already given some feedback that I think is quite good, so I'll avoid touching on the same subjects.

Summation

I'd go a step further with the summing through. The standard library already has std::accumulate to sum the results of a function applied to a range. I'd probably use the xrange class I posted in an earlier answer to generate a range of numbers that can be iterated like a range of values in a container (but without actually using a container to do it).

User input

Rather than telling the user they have to enter the lower end of the range first (and especially, rather than trusting them to follow the directions), I'd get the values, and swap them if they're out of order (and possibly warn the user if swapping was needed).

So, the code would look something like this:

#include "xrange"
#include <cmath>
#include <numeric>
#include <iostream>
#include <string>

int get_value(std::string const &prompt) {
    int val;
    std::cout << prompt << ": ";
    std::cin >> val;

    return val;
}

int main(int argc, char **argv) {
    int lower = get_value("Please enter lower end of range");
    int upper = get_value("Please enter upper end of range");
    int power = get_value("Please enter exponent");

    if (lower > upper)
        std::swap(upper, lower);

    auto range = xrange(lower, upper + 1 );

    auto sum = std::accumulate(range.begin(), range.end(),
        0,
        [=](int a, int b) { return a + (int)std::pow(b, power); });

    std::cout << "Sum is: " << sum << "\n";
}

Note that xrange creates a "half open" range (i.e., going from the lower end to one less than the upper end) so we add 1 to the upper end here, since I'd guess the typical user expects the range to be inclusive on both ends.

As it stands right now, this doesn't attempt to ask you whether to quit or carry out another calculation--if you really need that, I'll leave that part to you.

\$\endgroup\$
0
\$\begingroup\$

Apart from what SirPython mention

Indentation

You should use 4 spaces to indent your code.

int integer_sum(int a, int b){
    int int_sum(0);
    while(a <= b){
        int_sum += a;
        a++;
    }
    return int_sum;
}

Closed forms

Many of this summations have a closed form therefore, a complexity of O(1).

#include <vector>
#include <functional>
#include <iostream>


std::vector<std::function<int(int)>> closed_form
{
    [](int N) { return N; },
    [](int N) { return (N * (N + 1)) / 2; },
    [](int N) { return (N * (N + 1) * (2 * N + 1)) / 6; },
    [](int N) { return closed_form[1](N) * closed_form[1](N); }

    // add as many as you want..
};


int sum_series_exponent(int from, int to, int power) 
{
    if (power < closed_form.size())
    {
        return closed_form[power](to)
               - closed_form[power](from - 1);
    }
    int sum = 0;
    for (; from <= to; ++from) 
    {
        sum+= pow(from, power);
    }
    return sum;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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