6
\$\begingroup\$

I'm ultra new to programming. I basically know only IFs, loops, variables. This is first time I used functions in my code and I'm not sure if I understand them correctly.

Basically it is a calculator that requests two natural positive numbers on input, and outputs it's addition, subtraction, multiplication, division and exponentiation. The only rule is: you can use only addition when calculating these expressions. I'm ultra new to programming. I basically know only IFs, loops, variables. This is first time I usentiation. It does not yet support: using '0' as input, using negative numbers as input, using floating point numbers as input, indivisible numbers.

My question is: can you please take a look at my code and just tell me if I made any mistakes that literally burn your eyes? What should I fix? What is especially bad?

#include <iostream>
using namespace std;

int addition(int a, int b)
{
    return a + b;
}

int subtraction(int a, int b)
{
    return a + (-b);
}

int multiplication(int a, int b)
{
    int result = a;
    for (int i = 1; i < b; i++)
        result += a;
    return result;
}

int division(int a, int b)
{
    int counter = 0;
    if (a % b == 0) {
        do {
            a = subtraction(a, b);
            counter++;
        } while (a > 0);
        return counter;
    }
    else
        return false;
}

int exponentiation(int a, int b)
{
    int result = 1;
    for (int i = 1; i <= b; i++)
        result = multiplication(result, a);
    return result;
}

int main()
{
    int numberA, numberB;
    cout << "Insert two positive natural numbers: ";
    cin >> numberA >> numberB;
    cout << "\nAddition: " << addition(numberA, numberB) << '\n';
    cout << "Subtraction: " << subtraction(numberA, numberB) << '\n';
    cout << "Multiplication: " << multiplication(numberA, numberB) << '\n';
    cout << "Division: " << division(numberA, numberB) << '\n';
    cout << "Exponentiation: " << exponentiation(numberA, numberB) << '\n';
    return 0;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ "Use only addition" seems to forbid using the % operator in your division function. And you really don't need to use % there. Using unary - also seems a bit questionable under the rules of the assignment, but they probably didn't actually mean to rule it out. \$\endgroup\$ – David K Dec 12 '17 at 17:24
  • \$\begingroup\$ @DavidK Alternatively, could be to learn about or exploit overflows. \$\endgroup\$ – Quelklef Dec 12 '17 at 19:21
  • \$\begingroup\$ Welcome to Code Review and welcome to programming! If you know conditional and loop expressions you already know what's necessary to theoretically write any possible program. All other language concepts (functions, objects/classes, exceptions, etc.) are just means to express programs in ways that make them more easily understandable and maintainable, i. e. more practical to write. \$\endgroup\$ – David Foerster Dec 12 '17 at 21:04
11
\$\begingroup\$

Namespaces

You almost never want to write using namespace std; - this is a very common code complaint on this site with C++. The two main reasons are:

  • it's good to know when you're using stuff from the standard library explicitly,
  • standard library names can collide with your own since they've picked the best names!

auto

Consider also using return type deduction here as your functions are fairly trivial, and the following would work:

auto addition(int a, int b) { return a + b; }

The compiler has no trouble working out the return type is clearly an integer, and this will apply to other cases. Another use of auto is in your copy in the multiplication function, for example,

auto multiplication(int a, int b){
    auto result = a;
    // rest of code
}

You can apply this to a lot of other statements in your code. Depending on your style preference and the importance you place on uniformity, you can use the uniform initialisation syntax, auto result{a}.


Division

There's a problem in your division function. It returns counter which is an integer, and the return type is an integer - that's fine. But it can return false.

This compiles, and returning false will be like returning zero, but just do that explicitly, your return type isn't bool.

I would also move the declaration of the counter variable to inside the if statement, since if that statement fails, you don't need it.

So, if the integers don't exactly divide each other, you don't want to handle that case and you return zero. But you don't check this when outputting to the user. They'll get zero, so I'd opt for an explicit message, since that can have multiple interpretations.


In C++, return 0 is not required explicitly in main, so omit it. There's also an argument against it along the lines of, if you do include it, it's suggesting main can fail and that there are potentially other error codes that one must check for.

\$\endgroup\$
  • 11
    \$\begingroup\$ I disagree. 1) const int in the function signature is useless for the caller and makes reading it harder, 2) the same goes to auto - it should rather be used when the return type is irrelevant (like something internal or implementation-defined or whatever), and not only if it can be deduced, - I see a signature and I want to know what does the function return, so why on Earth do I have to deduce it from the function implementation? \$\endgroup\$ – lisyarus Dec 12 '17 at 18:57
  • 3
    \$\begingroup\$ In a call by value, const tells nothing to the caller: the argument gets copied, so it is irrelevant to the caller whether this copy is modified or not, - it's a copy anyway, not his original data. I agree that it is a good practice to add const whenever it applies, however I strongly feel that signatures are a wrong place for it. It is more a self-protection for the function author. (and yes, for complex function signatures even a single const can make a huge difference in readability). \$\endgroup\$ – lisyarus Dec 12 '17 at 19:38
  • 1
    \$\begingroup\$ I know, there are many people saying we should put auto everywhere, and others saying we should constrain the use greatly. In this particular case we gain nothing. Were it a template function - definitely, auto would be the way to go, - but it isn't a template. Yet, since your wording it suggestive, I agree that this doesn't deserve any more discussion. (P.S. All your other suggestions are very good!) \$\endgroup\$ – lisyarus Dec 12 '17 at 19:42
  • 1
    \$\begingroup\$ I won't comment on whether to use auto or not, but a lot of people unfortunately do not use a C++14 enabled compiler, so having automatic type deduction from functions without a trailing type will not work. Just an FYI. \$\endgroup\$ – Rakete1111 Dec 12 '17 at 20:00
  • 1
    \$\begingroup\$ @Rakete1111 I’m aware of those people and they have my condolensces. \$\endgroup\$ – JNS Dec 12 '17 at 20:56
1
\$\begingroup\$

Given the 'new to programming', I'm gonna forgive you for a couple of elements. Using using namespace will bite you in the end, though, until you learn about namespaces you should not worry about that.

multiplication

Your multiplication method is too complicated for what it has to be. You stated you did not support 0, though, that doesn't mean your code should have this assumption everywhere.

int multiplication(int a, int b)
{
    int result = 0;
    for (int i = 0; i < b; i++)
        result += a;
    return result;
}

Having a for-loop which starts at zero and goes to the end will cover the zero case. However, it will also be a pattern you will recognize a lot in the future. By indexing from 1, you deviate from that pattern and this code looks like it does something special, while it ain't needed.

division

Your division method contains some bugs. Most importantly: return false;. The method only has a single return type int so false will be implicitly converted to 0. The current code gives the impression that it is possible to check for failure.

int division(int a, int b)
{
    if (a % b != 0)
        return -1; // Failure

    int counter = 0;
    while (a > 0)
    {
        a = subtraction(a, b);
        ++counter;
    }
    return counter;
}

In the code as I wrote it, you will see a few changes:

  • Consistent use of {}, this allows scanning the code without having to think about the details.
  • Early return, this makes it more clear that you have elements in your function: Check on correctness + logic
  • Changed do-while to while, this covers again the zero case.
  • Use ++counter instead of counter++ as pre-increment is by design a more performant operation than post-increment. It might not be important in this case, though, it is a good habit to use the pre-increment by default.

main

I don't have many remarks, except for the cout << "\nAddition: ". I would have split this up in 2 parts:

cout << '\n';

cout << "Addition: " ...;

This way, you don't have consider this new-line in case the order should be changed.

You might also note the new-line between the two streaming operators, which can be used to visually separate the input and output block.

overal remarks

Overal, this code is very good given that you are a beginner.

As mentioned before, using namespace is evil. Beside this, you don't have error handling, so if the user inputs his/her name instead of the numbers, you will be in trouble. (PS: My return -1; might also not be the best solution, though, I suppose you haven't reached exceptions yet)

\$\endgroup\$

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.