Terminal calculator that saves and displays calculation history

Question

I am self-learning C++ and this was my first smallish project. The goal was to create a working calculator that runs in the terminal, saves history of previous calculations, and has the ability to enter commands to display history, re-display commands, and quit. The display history/command commands need to work at any point while entering values for a calculation without disrupting the calculation, and the quit command needs to end the whole program right after it is pressed. I wanted proper exception handling for incorrect input and (lower vs uppercase) command characters. Additionally, I had the goals of wanting the terminal UI to be easy-to-use and readable, and to use good naming conventions in my code.

Some questions I have:

• Is there a better/cleaner way to [Q]uit without having multiple if-statements to check the s_run flag?
• Is there a better/cleaner way to check if cin input was a command or double in the setNum method?
• I made the class a singleton since I only need one instance of the calculator class. Was this a good choice, and did I implement this right? First time creating a singleton.

Additional notes:

• I may add more features to the calculator, which is why I have the displayCommand command even though there are only 3 commands currently.
• I made this project with the idea of potentially making a small text-based adventure game in the terminal and wanted a way to use a calculator in-game without leaving the terminal.

Calculator.hpp:

#pragma once

#include <iostream>         //std::cout, std::cin
#include <iomanip>          //std::setprecision
#include <vector>           //std::vector, std::vector::push_back
#include <cctype>           //std::toupper
#include <string>           //std::string, std::string::strtod(), for setNum input commands

struct Calculation
{
    double n1;
    char op;
    double n2;
    double ans;
};

class Calculator
{
public:
    Calculator(const Calculator&) = delete;                 //Delete the copy constructor so that multiple instances of the singleton are not created
    static Calculator& get();                               //get function used to get single Singleton instance of Calculator class
    static void calculate() {return get().i_calculate();}   //Main member function, calls internal calculate function on the singleton instance

private:
    Calculation m_calc;                     //Struct of current calculation, holding n1, op, and n2 values for calculating + ans for answer value
    std::vector<Calculation> m_history;     //vector of calculation history
    inline static bool s_run;               //static flag to keep calculator while-loop running until [Q]uit
    static Calculator s_Instance;           //Singleton instance used for Calculator

private:
    Calculator() : m_calc({0,'+',0,0}) {};  //Default constructor, should not be called since Calculator is a Singleton, initialize m_calc Calculation struct
    void i_calculate();                     //Internal: Main member function that runs until Calculator object is destroyed
    void setAll();                          //sets n1, op, and n2 by calling setNum and setOp functions
    void setNum(double&);                   //sets Calculation n1 or n2 values, only excepts numbers
    void setOp(char&);                      //sets Calculation operator value, only excepts ('+' || '-' || '*' || '/')
    bool is_valid_command(const char&);     //returns true if char is a command character: [Q]uit, [H]istory, or [C]ommands
    bool is_valid_op(const char&);          //returns true if char is an calculator operator ('+' || '-' || '*' || '/')
    void command(const char&);              //Implements command corresponding to char parameter: [Q]uit, [H]istory, or [C]ommands
    void displayOperators() const;          //Displays operators to be used in calculator
    void displayAnswer() const;             //Displays "n1 op n2 =" and the answer of the calculation
    void displayHistory() const;            //Displays previous calculations
    void displayLine() const;               //prints a long "------" line to terminal, used for visual visibility
    void displayInfo() const;               //prints info about how calculator works
    void displayCommands() const;           //prints list of commands
    void startUp();                         //reset m_run flag to true, display calculator info
    void shutDown();                        //clear m_history and terminal
};

Calculator.cpp:

//Basic calculator in C++ that can perform addition, subtraction, multiplication, and division
#include "Calculator.hpp"   //Calculator class, Calculation struct        

Calculator& Calculator::get()
{
    static Calculator s_Instance;
    return s_Instance;
}

void Calculator::i_calculate()
{
    startUp();

    while(s_run == true)    //run calculations until [Q]uit is called
    {
        setAll();           //prompt user for n1, op, and n2 values

        switch(m_calc.op)
        {
            case '+':
                m_calc.ans = m_calc.n1 + m_calc.n2;
                break;
            case '-':
                m_calc.ans = m_calc.n1 - m_calc.n2;
                break;
            case '*':
                m_calc.ans = m_calc.n1 * m_calc.n2;
                break;
            case '/':
                if(m_calc.n2 != 0)
                    m_calc.ans = m_calc.n1 / m_calc.n2;
                else m_calc.ans = 0;
                break; 
        }

        if(s_run == true)
        {
            displayAnswer();                //display calc + ans on terminal
            m_history.push_back(m_calc);    //add calculation to history
        }
    }

    shutDown();
}

void Calculator::setAll()
{
    std::cout << "Enter first number: ";
    setNum(m_calc.n1);

    if(s_run == true)
    {
        displayOperators();
        std::cout << "Enter operation: ";
        setOp(m_calc.op);
    }

    if(s_run == true)
    {  
        std::cout << "Enter second number: ";
        setNum(m_calc.n2);
    }
}

void Calculator::setNum(double& n)
{
    std::string input;      //string for testing if input is a double or char
    char* c_ptr = nullptr;  //char pointer, will point to first char that cannot be converted to a double

    do
    {
        std::cin >> input;                              //get user input as a string
        double d_test = strtod(input.c_str(), &c_ptr);  //test if string is a double by using strtod. If double, c_ptr == nullptr
        if(*c_ptr == 0)                                 //if a double
        {
            n = d_test;     //set n to double
            break;          //break do-while loop
        }
        else if(input.length() == 1)        //if string is only a char
        {
            if(is_valid_command(*c_ptr))    //if the char is a command
            {
                command(*c_ptr);            //implement command
                if(s_run == false)          //if command was [Q]uit
                    break;                  //break do-while loop
            }
        }
        else    //Invalid input: not a double or a command char
        {
            std::cout << "Error: not a number" << std::endl;
            std::cin.clear();                                   //clears the error flag on cin
            std::cin.ignore(256, '\n');                         //skip to the next new line
        }

        std::cout << "Enter number: ";                          //ask for new number

    } while (true);
}

void Calculator::setOp(char& ch)
{
    do
    {
        std::cin >> ch;
        if(is_valid_command(ch))        //check if char is a command
            command(ch);                //if a command, implement command
        else if(!(is_valid_op(ch)))                             //check if ch is an operator char (+,-,*,/)
        {
            std::cout << "Error: not an operator" << std::endl;
            std::cin.clear();                                   //clears the error flag on cin
            std::cin.ignore(256, '\n');                         //skip to the next new line
        }
        else break;                                             //end do-while loop once a valid char has been entered

        std::cout << "Enter operation: ";                       //ask for new operator

    } while (true);  
}

bool Calculator::is_valid_command(const char& ch)
{
    return (std::toupper(ch) == 'Q')
        || (std::toupper(ch) == 'H')
        || (std::toupper(ch) == 'C');
}

bool Calculator::is_valid_op(const char& ch)
{
    return ch == '+'
        || ch == '-'
        || ch == '*'
        || ch == '/';
}

void Calculator::command(const char& ch)
{
    if(std::toupper(ch) == 'Q')                             //if Q was entered, shut down calculator
    {
        s_run = false;                                      //set run flag to false for calculate do-while-loop
    }
    else if(std::toupper(ch) == 'H')                        //if H was entered, display calculation history
        displayHistory();
    else if(std::toupper(ch) == 'C')                        //if C was entered, display commands list
        displayCommands();
}

void Calculator::displayOperators() const
{
    std::cout << "Addition: + | Subtraction: - | Multiplication: * | Division: /" << std::endl;
}

void Calculator::displayAnswer() const
{
    system("clear");    //clear the terminal before displaying answer
    displayLine();
    std::cout << m_calc.n1 << " " << m_calc.op << " " << m_calc.n2 << " =" << std::endl;
    std::cout << m_calc.ans << std::endl; 
    displayLine();
}

void Calculator::displayHistory() const
{
    displayLine();

    if(m_history.empty())
        std::cout << "No calculations have been done yet." << std::endl;
    else
    {
        for (const Calculation& struct_i : m_history)       //ranged-based for-loop, iterate over each Calculation struct
        {
            std::cout << struct_i.n1 << " " << struct_i.op << " " << struct_i.n2 << " =" << std::endl;
            std::cout << struct_i.ans << std::endl;
            displayLine();
        }
    }
}

void Calculator::displayLine() const
{
    std::cout << "--------------------------------------------" << std::endl;
}

void Calculator::displayInfo() const
{
    std::cout << "Terminal controllable calculator that does (+,-,*,/) operations and holds a history of calculations" << std::endl;
}

void Calculator::displayCommands() const
{
    std::cout << "Commands: [Q]uit, [H]istory, [C]ommands" << std::endl;
}

void Calculator::startUp()
{
    std::cout << "Calculator starting up..." << std::endl;
    displayInfo();
    displayCommands();
    s_run = true;                   //reset s_run if reusing calculator object
    std::cout.precision(16);        //set precision to 16 decimal places max
}

void Calculator::shutDown()
{
    m_history.clear();      //clear Calculation vector history
    system("clear");        //clear terminal
    std::cout << "Calculator shutting down..." << std::endl;
}
```

Answer 1

Here are some things that may help you improve your code.

Avoid singletons

You probably already know about avoiding global data because it makes the program harder to understand and reason about. The same is true of singletons, and there is the additional problem in C++ that singletons constructed in this way are not thread safe. There isn't really any compelling reason that one couldn't have multiple Calculator instances in this program, and it could easily be made thread safe.

Avoid system("clear")

Using system is not really necessary most of the time and it creates a dependency on both an external program and on a specific platform. For example, if your terminal supports ANSI Escape sequences, you could use this:

void clearScreen()
{
    std::cout << "\x1b[2J";
}

Separate input, output and calculation

To the degree practical it's usually good practice to separate input, output and calculation for programs like this. By putting them in separate functions, it isolates the particular I/O for your platform (which is likely to be unique to that platform or operating system) from the logic of the program (which does not depend on the underlying OS). For instance, consider this function:

void Calculator::setOp(char& ch)
{
    do
    {
        std::cin >> ch;
        if(is_valid_command(ch))        //check if char is a command
            command(ch);                //if a command, implement command
        else if(!(is_valid_op(ch)))                             //check if ch is an operator char (+,-,*,/)
        {
            std::cout << "Error: not an operator" << std::endl;
            std::cin.clear();                                   //clears the error flag on cin
            std::cin.ignore(256, '\n');                         //skip to the next new line
        }
        else break;                                             //end do-while loop once a valid char has been entered

        std::cout << "Enter operation: ";                       //ask for new operator

    } while (true);  
}

This is some convoluted code that does multiple things: it gets input from the user, validates that input and handles errors. Better would be to separate these concerns into reusable components. For example, in multiple places, the code gets input from the user and then validates that against some rules. Why not make that pattern explicit and generic?

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Prefer a stream inserter to a custom print routine

The code writes out the calculation in two places: in the displayHistory and the displayAnswer functions. Instead, that could be written as a stream inserter once and then used in both places if needed:

std::ostream& operator<<(std::ostream& out, const Calculation& calc) {
    return out << calc.n1 << " " << calc.op << " " << calc.n2 << " =\n" 
        << calc.ans << "\n--------------------------------------------";
}


void Calculator::displayHistory() const {
    displayLine();
    if (m_history.empty()) {
        std::cout << "No calculations have been done yet.\n";
    } else {
        std::copy(m_history.begin(), m_history.end(), std::ostream_iterator<Calculation>(std::cout, "\n"));
    }
}

Similarly, it seems to me that the Calculation object should be responsible for actually performing the calculation. One could use something like this:

double Calculation::operator()() {
    switch(op)
    {
        case '+':
            ans = n1 + n2;
            break;
        case '-':
            ans = n1 - n2;
            break;
        case '*':
            ans = n1 * n2;
            break;
        case '/':
            if(n2 != 0)
                ans = n1 / n2;
            else ans = 0;
            break; 
    }
    return ans;
}

Consider using a menu object or at least a common menu function

In a number of places in your code, you have something like a menu. Your code presents a couple of options and then asks the user to pick one. Rather than repeating that code in many places, it would make sense to make it generic. Only the prompt strings actually change, but the underlying logic of presenting the choices and asking for input are all the same.

Use return values for returning values

The code currently contains this function:

void Calculator::setOp(char& ch)
{
    do
    {
        std::cin >> ch;
        if(is_valid_command(ch))        //check if char is a command
            command(ch);                //if a command, implement command
        else if(!(is_valid_op(ch)))                             //check if ch is an operator char (+,-,*,/)
        {
            std::cout << "Error: not an operator" << std::endl;
            std::cin.clear();                                   //clears the error flag on cin
            std::cin.ignore(256, '\n');                         //skip to the next new line
        }
        else break;                                             //end do-while loop once a valid char has been entered

        std::cout << "Enter operation: ";                       //ask for new operator

    } while (true);  
}

Here is how I would write that instead:

char Calculator::getOp()
{
    char ch;
    for (std::cin >> ch; !Calculation::isValidOp(ch); std::cin >> ch) {
        std::cout << "Error: not an operator\n";
        std::cin.clear();                                   //clears the error flag on cin
        std::cin.ignore(256, '\n');                         //skip to the next new line
    }
    return ch;
}

This returns the newly input and validated operator character, and it also moves the isValidOp() function to be a static member of the Calculation class, which is where it really belongs.

Answer 2

For a first project in C++, this is looking really decent! There are still some things you can improve though, which I'll list below.

Answers to your questions

• Is there a better/cleaner way to [Q]uit without having multiple if-statements to check the s_run flag?

Yes, by reading a whole expression in one go, instead of asking for the first number, operation, and second number separately. I'll elaborate below.

• Is there a better/cleaner way to check if cin input was a command or double in the setNum method?

Yes, again by reading a whole expression or command in one go.

• I made the class a singleton since I only need one instance of the calculator class. Was this a good choice, and did I implement this right? First time creating a singleton.

This was not a good choice. There is nothing inherently preventing the code from working fine if there are multiple Calculator instances (apart from s_run which should not have been made static). If the code that uses the Calculator class only wants a single instance, they can just declare a single one.

Reserve the singleton pattern for objects of which there really can only be one. Don't use it merely because you cannot think of anyone wanting to have two instances at the same time.

However, if you did need a singleton, then your implementation almost correct. You should also delete the assignment operator. See this example of a singleton class in C++.

Consider not using a class

Perhaps you already know how to program in Java, where everything has to be in a class, but in C++ you can have stand-alone functions that are not in a class. Your header file could be made to look like:

#pragma once

void calculate();

And your source file like so:

static bool running;

static void startUp() {
    ...
}

/* ... The rest of the private functions and variables, all made static. */

void calculate() {
    startUp();

    while(running)
    {
        ...
    }

    shutDown();
}

Separate the UI from the logic

Your Calculator is doing many things; it is dealing with input and output to the console (the UI), doing the parsing and evaluation of expressions, and keeping a history of past calculations. It starts to look like a God object. Instead, try to separate it into smaller and simpler components. In particular, separate the user interface from the actual calculation.

Simplify the input handling

A significant amount of complexity comes from the way you handle input. Reading the numbers and operator separately using std::cin >> input might seem a good idea at first, but it has all sorts of issues. As you've already encountered, now you have to deal with errors at each part of the input, requiring you to check s_run multiple times, but also having to call std::cin.clear() and std::cin.ignore() to flush the input buffer. The latter functions will almost always result into problems. For example, what if someone typed more than 256 characters before a newline?

The best way would be to let the user enter one command or one expression per line. You can read a whole line into a std::string using std::getline(), and then parse that string however you see fit without having to worry about the state of std::cin. For example:

std::string line;
std::cout << "Enter a command or an expression:\n";

if (!std::getline(std::cin, line)) {
    /* Error while reading input, just quit */
    ...
}

if (line == "Q") {
    /* Handle the quit command */
} else if (line == "H") {
    ...
} else {
    parse_expression(line);
}

Where parse_expression() could look like:

auto parse_expression(const std::string& expression) {
    std::stringstream ss(expression);
    Calculation calc;
    ss >> calc.n1 >> calc.op >> calc.n2;

    if (!ss) {
        /* Error while parsing the expression */
        ...
    }

    ...
}