7
\$\begingroup\$

I need someone with experience to judge my first project to let me know what I have to work on. I recently started learning C++ using resources like Solo Learn and Bucky's tutorials on YouTube hoping to eventually gain enough experience to find a job in software development.

#include <iostream>
#include <string>
using namespace std;
//Functions



// addition function
int calc_sum(){
double x;
double y;
cout << "enter two numbers to get a sum" <<endl;
cin >> x >> y;
double sum = x+y;
cout <<"The sum is "<< sum <<"."<<endl;

}
// multiplication function
int calc_pro(){
double a;
double b;
cout<<"enter two numbers to find the product"<<endl;
cin>> a >> b;
double product = a * b;
cout<< "The product is "<< product <<"."<<endl;

}
// division function
int calc_div(){
double x;
double y;
cout<< "enter two numbers to divide."<<endl;
cin>> x >> y;
double div = x/y;
cout << "The quotient is "<< div <<"." <<endl;
}
// Subtraction function
int calc_sub() {
double a;
double b;
cout<< "Enter two numbers to subtract."<< endl;
cin>> a >> b;
double diff = a-b;
cout<< "The difference is "<< diff <<endl;

}

//core function

void core2(){
string choice;
cout<<"Would you like to multiply, divide, add, or subtract? (typer in lowercase)"<<endl;
cin>>choice;

if(choice=="add"){
calc_sum();
core2();
}
else if(choice=="subtract"){
calc_sub();
core2();
}
else if(choice=="multiply"){
calc_pro();
core2();
}
else if(choice=="divide"){
calc_div();
core2();
}
else{
cout<<"USER ERROR"<<endl<< "you typed in something wrong, try again."<<endl;
core2();
}
}







int main(){


core2();



}
\$\endgroup\$

closed as off-topic by πάντα ῥεῖ, VisualMelon, Sᴀᴍ Onᴇᴌᴀ, Heslacher, Donald.McLean Feb 5 at 13:45

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – πάντα ῥεῖ, Sᴀᴍ Onᴇᴌᴀ, Donald.McLean
If this question can be reworded to fit the rules in the help center, please edit the question.

12
\$\begingroup\$

Code formatting

Format your code to improve readability and hence maintainability.

DRY (Don't Repeat Yourself) Principle

You have a lot of places with duplicate code. You should try to avoid this.

First of all in your core2 method you have calls in all condition blocks, you should move it below to be executed in any case.

Also you have many duplicate code in your calc methods. You can move duplicate code to your core core2 method.

Inconsistent styles

Use the same style everywhere. If you are starting your sentences for output with uppercase then try to follow it everywhere. The same is true about comments and constructs like to get a sum, to find a product, to divide. Use single ubiquitous style for everything.

Function return types

If you don't want to return anything from your function it should be void. All your calc functions are ints (but you don't call return operator) and you are not using return values. So all your calc functions should be void.

Redundant core2 function

Also core2 function is not needed here, you can use main directly. But C++ specs prohibits this behaviour although this works fine in most compilers. So we can replace core2 recursion with infinite loop in main.


So if we take into account all above notes code will look like this

#include <iostream>
#include <string>
using namespace std;

// Now we have only primitive one-liner arithmetic functions
// with double return types    

double add(double a, double b) { return a + b; }

double subtract(double a, double b) { return a / b; }

double multiply(double a, double b) { return a * b; }

double divide(double a, double b) { return a / b; }

void core2() {
    string choice;
    cout << "Would you like to multiply, divide, add, or subtract? (typer in lowercase)" << endl;
    cin >> choice;

    // Choosing correct action name to output
    string result_name;

    if (choice == "add") {
        result_name = "sum";
    } else if (choice == "subtract") {
        result_name = "difference";
    } else if (choice == "multiply") {
        result_name = "product";
    } else if (choice == "divide") {
        result_name = "quotient";
    } else { // This block also performs validation of input parameter
        result_name = ""; // Just to prevent compiler error due to undeclared variable
        cout << "USER ERROR" << endl << "you typed in something wrong, try again." << endl;
        return; // end execution here
    }

    // Entering numbers in single place
    double a;
    double b;
    cout << "Enter two numbers to get the "<< result_name << "." << endl;
    cin >> a >> b;

    double result;

    if (choice == "add") {
        result = add(a, b);
    } else if (choice == "subtract") {
        result = subtract(a, b);
    } else if (choice == "multiply") {
        result = subtract(a, b);
    } else {
        result = divide(a, b);
    }

    // Outputting result in single place
    cout << "The " << result_name << " is " << result << "." << endl;
}

int main() {
    while (true) {
        core2();
    }
}

Further improvement will use more advanced techniques and relatively new C++11 features.

You need some way to map string and function, so for this we will use std::map as dictionary and std::function as polymorphic function wrapper. Also lambdas will be use to pass anonymous functions here as parameters. struct will be here used as container to store calculation function and result description.

#include <iostream>
#include <string>
#include <functional>
#include <map>
using namespace std;

struct CalculationTechnique {
    std::function<double (double,double)> Function;
    string ResultName;
};

void core2() {
    string choice;
    cout << "Would you like to multiply, divide, add, or subtract? (typer in lowercase)" << endl;
    cin >> choice;

    // Filling our dictionary
    std::map<string, CalculationTechnique> calculation_mapping = {
        { "add", CalculationTechnique { [](double a, double b) -> double { return a + b; }, "sum" } },
        { "subtract", CalculationTechnique { [](double a, double b) -> double { return a - b; }, "difference" } },
        { "multiply", CalculationTechnique { [](double a, double b) -> double { return a * b; }, "product" } },
        { "divide", CalculationTechnique { [](double a, double b) -> double { return a / b; }, "quotient" } }
    };    

    // if no value found in dictionary
    if (calculation_mapping.count(choice) == 0) {
        cout << "USER ERROR" << endl << "you typed in something wrong, try again." << endl;
        return;
    }

    CalculationTechnique technique = calculation_mapping.at(choice);

    double a;
    double b;
    cout << "Enter two numbers to get the "<< technique.ResultName << "." << endl;
    cin >> a >> b;

    cout << "The " << technique.ResultName << " is " << technique.Function(a, b) << "." << endl;
}

int main() {
    while (true) {
        core2();
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Now that is an answer I was looking for, helpful and with example. I'm not sure how long this took you to write, but I'm really glad there are people like you. There is indeed a lot of things I have to learn, do you know any resources that can be helpful on my journey to a career in software development? I am self taught and there's only so much you can do a website like sololearn. \$\endgroup\$ – JuanR Feb 2 at 10:48
  • 2
    \$\begingroup\$ @JuanR I'm self taught also. Answer writing took about 1,5-2 hours. I'm not big expert at C++, just studied some time before completely switched to C#. Best resources should have a lot of tasks for you to practice, therefore my favourite C++ book is "C++ How to Program" by Deitel and Deitel because it has a lot of tasks in the end of every chapter. I even used this tasks when studying other programming languages. Obviously programmer is learning programming by creating programs, so I dislike books that just talk about programming and don't have tasks to do. \$\endgroup\$ – Vadim Ovchinnikov Feb 2 at 11:02
  • 1
    \$\begingroup\$ @JuanR Also learn fresh language features (C++11, C++14), don't use outdated resources that don't use them. C++ is also hard language because it has manual memory management, a lot of hard concepts and syntax pitfalls, it has a lot of rubbish syntax heritage from C. If you like programming but not very huge fan of C++, consider studying some other languages (except you want to develop triple A games, hardcore 3D or low-level software). General advice is just to study requirements for your desired job. \$\endgroup\$ – Vadim Ovchinnikov Feb 2 at 11:10
  • 1
    \$\begingroup\$ Remove your recursive main() call and I'll up vote. \$\endgroup\$ – bruglesco Feb 2 at 17:05
  • 1
    \$\begingroup\$ Storing result_name into a variable and then using that in the middle of a sentence is definitely not good code from localization perspective. You have to localize the whole sentence as one unit. \$\endgroup\$ – Sulthan Feb 2 at 20:06
5
\$\begingroup\$

Looking at the code, I can't deduce if you are learning C with some sparkles of C++ or actual C++. So I'm curious to see the continuation.

That said, let me look at the actual code. I don't like using namespace std;, though for beginners I can agree using it might make things easier so you can focus on the other stuff first.

Looking further, I see an inconsistent use of types in calc_sum. You nicely use doubles for the input, and than you return an int. So, you can enter: 0.1 and 0.8, and the result will be 0. I suggest you read up about the difference between int and double to find out why 0 is returned.

Looking closer, you don't return anything. So you are actually in UB land (undefined behavior). Which makes me realize that your implementation is surprising me. Part of the explanation above simply doesn't hold as I read in patterns.

Reading through, I see a method called calc_pro, (from product?). With documentation about the multiplication. Here you have fallen in a trap I still see with senior developers: Don't use abbreviations in function names, these will conflict over time. Also, don't spare characters. With a good IDE or text editor, you get auto complete. Be descriptive.

Next up, you have discovered recursion. core2 calls itself. On its own, not a bad thing, though not appropriate here. Given enough input, your program will crash. Using a do-while or a regularwhile sounds like a better solution.

At one point, you also print an error to cout, the output stream. There is however an cerr which is an error stream, something a console could color differently.

Finally, there is a lot of copy paste and functions doing too much at once. Every function requests 2 arguments from cin, does a calculation and prints to cout. This should be 3 functions.

std::pair<double, double> getNumbers(string action) {
    double a;
     ...
      cout << "Enter ... to " << action << "." << endl;
     ...
     return { a, b };
 }

And so on.

\$\endgroup\$
4
\$\begingroup\$

Indentation

You should indent your methods and if/while blocks in order to improve general readability.

Unnecessary Recursion

If your user decides to perform enough operations, it could result in the call stack overflowing and affecting other applications. In your application, a loop is a more appropriate choice than recursion.

DRY - Don't Repeat Yourself

You have four different places where you are getting inputs. While it may be OK on this occasion (it's only one line after all), if it were any longer I would suggest extracting it to a method to allow for easy reuse.

Typo

typer in lowercase should be type in lowercase.

\$\endgroup\$
  • \$\begingroup\$ Can you tell me which lines are considered to have recursion? \$\endgroup\$ – JuanR Feb 2 at 8:43
  • 4
    \$\begingroup\$ Any time you're calling core2() within the core2() method. \$\endgroup\$ – Joe C Feb 2 at 9:01

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