8
\$\begingroup\$

Premise

I'm writing a simple postfix calculator app in C++ using the Qt (5) GUI framework. This is simply for practice with software development.

Visually, my calculator looks like this:

enter image description here

The calculator has a Back button (with a corresponding keyboard shortcut) that allows users to erase the previous input.

The code

All of the code works properly, but only parts of it are relevant to this question. I will post relevant snippets here to make things easier to understand.

Restricting user input

At each stage of input, I restrict the kinds of actions that users can take. For example, at the start of the application, the user may enter a digit or operator but may not enter an opening or closing parenthesis.

If a user enters an operator, such as +, they may enter a digit or opening parenthesis but may not enter another operator or closing parenthesis.

These restrictions are applied within the code that takes care of what happens when a user pushes the corresponding button for a digit or operation. For example, if a user pushes a digit, the code that handles the release of a digit button will first check if a digit is allowed before proceeding to update the input text to reflect the user's input.

Issue: reverting to previous input while retaining restrictions

When the user presses and releases the backspace key, the corresponding code needs to not only delete the most recent character that was entered but also revert the current "state" of the calculator to previous restrictions. In other words, if the expression is originally:

1 + 2

And the user presses the backspace key, the new expression will be:

1 +

And the calculator must therefore enforce any restrictions that apply after an operator has been entered. In other words, after hitting the backspace key, the user can no longer enter another operator and must either enter an opening parenthesis or a digit.

My approach

I chose to implement this functionality using the following struct:

struct State
    {
        bool digitAllowed;
        bool operatorAllowed;
        bool openParenthAllowed;
        bool closingParenthAllowed;
        int numOpenParenths;
        int numClosingParenths;
        State(bool dig, bool op, bool oP, bool cP, int nO=0, int nC=0) :
            digitAllowed(dig), operatorAllowed(op),
            openParenthAllowed(oP), closingParenthAllowed(cP),
            numOpenParenths(nO), numClosingParenths(nC) {}
        State() {}
    };

This can be found in the following header (mainwindow.h):

#ifndef MAINWINDOW_H
#define MAINWINDOW_H

#include <QMainWindow>
#include <QPushButton>
#include <QLabel>
#include <QKeyEvent>
#include <QStack>

namespace Ui {
class MainWindow;
}

class MainWindow : public QMainWindow
{
    Q_OBJECT

    /* Used to keep track of the history of operations that the user
     * performed when entering input. Primarily used by backspace and
     * clear to undo previous user input and restore the calculator's
     * state to its previous input restrictions.
     */
    struct State
    {
        bool digitAllowed;
        bool operatorAllowed;
        bool openParenthAllowed;
        bool closingParenthAllowed;
        int numOpenParenths;
        int numClosingParenths;
        State(bool dig, bool op, bool oP, bool cP, int nO=0, int nC=0) :
            digitAllowed(dig), operatorAllowed(op),
            openParenthAllowed(oP), closingParenthAllowed(cP),
            numOpenParenths(nO), numClosingParenths(nC) {}
        State() {}
    };

public:
    explicit MainWindow(QWidget *parent = 0);
    Ui::MainWindow* getUI() const { return ui; }
    ~MainWindow();

private:
    Ui::MainWindow *ui;
    QLabel *input;
    QStack<State> history;
    bool operatorUsedDirectlyBefore() const;
    void reset();

signals:
    void input_is_ready(QString input);

private slots:
    void on_digit_released();
    void on_buttonDecimalPoint_released();
    void on_buttonNegate_released();
    void on_binary_button_released();
    void on_buttonEquals_released();
    void on_buttonClear_released();
    void on_buttonRoot_released();
    void on_buttonOpenParenth_released();
    void on_buttonCloseParenth_released();
    void on_buttonXSquared_released();
    void on_buttonSqrt_released();
    void on_buttonBack_released();
};

#endif // MAINWINDOW_H

Notice that my mainwindow then has a private stack of States:

QStack<State> history;

As the user provides input, the code creates a new State object and pushes it onto the stack. Here's the .cpp file (there are TODO statements for things I want to do in the future; please ignore those):

#include "mainwindow.h"
#include "ui_mainwindow.h"
#include "operator.h"
#include <QDebug>               // debug output
#include <QChar>
#include <QShortcut>            // keyboard input
#include <QKeySequence>         // keyboard input
#include <QFont>                // Qt fonts


/* Constructor for MainWindow objects. Connects all button signals
 * to their appropriate private slots to handle user input and sets
 * up some basic member variables.
 */
MainWindow::MainWindow(QWidget *parent) :
    QMainWindow(parent),
    ui(new Ui::MainWindow)
{
    // Basic variable setups
    ui->setupUi(this);
    input = ui->labelInput;
    reset();

    // Connect digits' released() signals to on_digit_released
    connect(ui->button0, SIGNAL(released()), this, SLOT(on_digit_released()));
    connect(ui->button1, SIGNAL(released()), this, SLOT(on_digit_released()));
    connect(ui->button2, SIGNAL(released()), this, SLOT(on_digit_released()));
    connect(ui->button3, SIGNAL(released()), this, SLOT(on_digit_released()));
    connect(ui->button4, SIGNAL(released()), this, SLOT(on_digit_released()));
    connect(ui->button5, SIGNAL(released()), this, SLOT(on_digit_released()));
    connect(ui->button6, SIGNAL(released()), this, SLOT(on_digit_released()));
    connect(ui->button7, SIGNAL(released()), this, SLOT(on_digit_released()));
    connect(ui->button8, SIGNAL(released()), this, SLOT(on_digit_released()));
    connect(ui->button9, SIGNAL(released()), this, SLOT(on_digit_released()));

    // Connect binary operators' released() signals to on_binary_button_released
    connect(ui->buttonPlus, SIGNAL(released()),
            this, SLOT(on_binary_button_released()));
    connect(ui->buttonMinus, SIGNAL(released()),
            this, SLOT(on_binary_button_released()));
    connect(ui->buttonTimes, SIGNAL(released()),
            this, SLOT(on_binary_button_released()));
    connect(ui->buttonDivide, SIGNAL(released()),
            this, SLOT(on_binary_button_released()));

    // Shortcuts for math input via keyboard
    // TODO potential memory leaks
    QShortcut *shortcut0 = new QShortcut(QKeySequence("0"), this);
    QShortcut *shortcut1 = new QShortcut(QKeySequence("1"), this);
    QShortcut *shortcut2 = new QShortcut(QKeySequence("2"), this);
    QShortcut *shortcut3 = new QShortcut(QKeySequence("3"), this);
    QShortcut *shortcut4 = new QShortcut(QKeySequence("4"), this);
    QShortcut *shortcut5 = new QShortcut(QKeySequence("5"), this);
    QShortcut *shortcut6 = new QShortcut(QKeySequence("6"), this);
    QShortcut *shortcut7 = new QShortcut(QKeySequence("7"), this);
    QShortcut *shortcut8 = new QShortcut(QKeySequence("8"), this);
    QShortcut *shortcut9 = new QShortcut(QKeySequence("9"), this);
    QShortcut *shortcutDecimal = new QShortcut(QKeySequence("."), this);
    QShortcut *shortcutPlus = new QShortcut(QKeySequence("+"), this);
    QShortcut *shortcutMinus = new QShortcut(QKeySequence("-"), this);
    QShortcut *shortcutTimes = new QShortcut(QKeySequence("SHIFT+8"), this);
    QShortcut *shortcutDivide = new QShortcut(QKeySequence("/"), this);
    QShortcut *shortcutOpenParenth = new QShortcut(QKeySequence("SHIFT+9"), this);
    QShortcut *shortcutCloseParenth = new QShortcut(QKeySequence("SHIFT+0"), this);
    QShortcut *shortcutEnter = new QShortcut(QKeySequence(Qt::Key_Return), this);
    QShortcut *shortcutBackspace = new QShortcut(QKeySequence("Backspace"), this);
    connect(shortcut0, SIGNAL(activated()), ui->button0, SLOT(click()));
    connect(shortcut1, SIGNAL(activated()), ui->button1, SLOT(click()));
    connect(shortcut2, SIGNAL(activated()), ui->button2, SLOT(click()));
    connect(shortcut3, SIGNAL(activated()), ui->button3, SLOT(click()));
    connect(shortcut4, SIGNAL(activated()), ui->button4, SLOT(click()));
    connect(shortcut5, SIGNAL(activated()), ui->button5, SLOT(click()));
    connect(shortcut6, SIGNAL(activated()), ui->button6, SLOT(click()));
    connect(shortcut7, SIGNAL(activated()), ui->button7, SLOT(click()));
    connect(shortcut8, SIGNAL(activated()), ui->button8, SLOT(click()));
    connect(shortcut9, SIGNAL(activated()), ui->button9, SLOT(click()));
    connect(shortcutDecimal, SIGNAL(activated()), ui->buttonDecimalPoint, SLOT(click()));
    connect(shortcutPlus, SIGNAL(activated()), ui->buttonPlus, SLOT(click()));
    connect(shortcutMinus, SIGNAL(activated()), ui->buttonMinus, SLOT(click()));
    connect(shortcutTimes, SIGNAL(activated()), ui->buttonTimes, SLOT(click()));
    connect(shortcutDivide, SIGNAL(activated()), ui->buttonDivide, SLOT(click()));
    connect(shortcutOpenParenth, SIGNAL(activated()), ui->buttonOpenParenth, SLOT(click()));
    connect(shortcutCloseParenth, SIGNAL(activated()), ui->buttonCloseParenth, SLOT(click()));
    connect(shortcutEnter, SIGNAL(activated()), ui->buttonEquals, SLOT(click()));
    connect(shortcutBackspace, SIGNAL(activated()), ui->buttonBack, SLOT(click()));
}

MainWindow::~MainWindow()
{
    delete input;
    delete ui;
}

/* Used by other functions to check if an operator was
 * applied directly before the current user's input token.
 */
bool MainWindow::operatorUsedDirectlyBefore() const
{
    return ((bool)operators.count(input->text().at(input->text().length()-1)));
}


/* Called when a user clicks any of the digit buttons (0-9) or
 * enters a digit from their keyboard (0-9). Appends the corresponding
 * digit to the input.
 */
void MainWindow::on_digit_released()
{
    State currentState = history.top();

    if(currentState.digitAllowed)
    {
        QPushButton *button = (QPushButton*)sender();

        history.push(State(true, true, false, true,
                                history.top().numOpenParenths,
                                history.top().numClosingParenths));

        // Initial input is just a 0
        if(input->text().length() == 1 && input->text()[0] == '0')
        {
            input->setText(input->text().replace(0, 1, button->text()));
        }
        // Otherwise, append the digit
        else
        {
            input->setText(input->text().append(button->text()));
        }
    }
}

/* Called when a user clicks the '.' button or uses the keyboard
 * shortcut '.' Appends a decimal point to the last digit that was
 * entered.
 */
void MainWindow::on_buttonDecimalPoint_released()
{
    // TODO prevent entry of multiple decimal points
    if(!operatorUsedDirectlyBefore())
    {
        input->setText(input->text() + ".");
    }
}

/* Called when a user clicks the '±' button. Negates an input expression.
 * Applies the negative sign to the last number entered (if one exists).
 * Negating a negative number will remove the negative sign. Negating the
 * right operand of addition will change the '+' operator to subtraction.
 * Similarly, negating the right operand of subtraction will change the
 * '–' operator to addition.
 */
void MainWindow::on_buttonNegate_released()
{
    // Check if there are currently any operators in input
    bool inputHasOperators = false;
    // If so, get the index of the last one (hence reverse traversal)
    int indexOfLastOperator = -1;
    for(int i = input->text().length() - 1; i >= 0; i--)
    {
        if(operators.count(input->text().at(i)) == 1)
        {
            inputHasOperators = true;
            indexOfLastOperator = i;
            break;
        }
    }

    // If we entered an operator and then tried to negate something
    if(indexOfLastOperator == input->text().length()-1){ return; }

    // Case 1: Input contains operators
    if(inputHasOperators)
    {
        QChar lastOperator = input->text().at(indexOfLastOperator);

        // If the number in question is already negative, undo it
        if(input->text().at(indexOfLastOperator+1) == '-')
        {
            input->setText(input->text().replace(indexOfLastOperator+1, 1, ""));
        }

        // If not, let's check what kind of operator came before it
        else
        {
            // If it was a plus, change that to a minus
            if(lastOperator == '+')
            {
                input->setText(input->text().replace(indexOfLastOperator, 1, "–"));
            }
            // If it was a minus, change that to a plus
            else if(lastOperator == '–')
            {
                input->setText(input->text().replace(indexOfLastOperator, 1, "+"));
            }
            // Otherwise, just negate the last number
            else
            {
                input->setText(input->text().insert(indexOfLastOperator+1, '-'));
            }
        }
    }

    // TODO or parenthetical expression?
    // Case 2: No operators and already negative number
    else if(input->text().at(0) == '-')
    {
        input->setText(input->text().replace(0, 1, ""));
    }

    // Case 3: No operators and positive number
    else
    {
        input->setText(input->text().prepend("-"));
    }
}

/* Called when a user clicks any binary operation's button
 * (+, -, *, /). Inserts the appropriate operation into input.
 */
void MainWindow::on_binary_button_released()
{
    QPushButton *button = (QPushButton*)sender();
    State currentState = history.top();

    if(currentState.operatorAllowed)
    {
        input->setText(input->text().append(button->text()));
        history.push(State(true, false, true, false,
                           currentState.numOpenParenths,
                           currentState.numClosingParenths));
    }
}

/* Called when a user clicks the '=' button or uses the keyboard
 * shortcut 'Enter'. Checks if the input is ready for evaluation.
 * If so, emits input_is_ready signal, effectively handing its
 * input string to the Calculator for processing.
 */
void MainWindow::on_buttonEquals_released()
{
    State currentState = history.top();
    if(!operatorUsedDirectlyBefore() &&
            currentState.numOpenParenths == currentState.numClosingParenths)
    {
        emit input_is_ready(input->text());
    }
}

/* Used to reset user input. Sets the input string to "0", clears its
 * history, and applies all initial input restrictions.
 */
void MainWindow::reset()
{
    input->setText("0");
    history.clear();
    history.push(State(true, true, false, false));
}

/* Called when a user clicks the 'Clear' button or uses the keyboard
 * shortcut 'Delete'. Resets the input. See MainWindow::reset().
 */
void MainWindow::on_buttonClear_released()
{
    reset();
}

/* Called when a user clicks the 'Back' button or uses the
 * keyboard shortcut 'Backspace'. Reverts the calculator to
 * its previous state of input. Reapplies all appropriate input
 * restrictions that existed prior to deletion of the last token.
 */
void MainWindow::on_buttonBack_released()
{
    // If the input is just a single digit, set it to 0
    if(input->text().length() == 1)
    {
        input->setText("0");
    }

    // Otherwise, if we have 2 or more entries, remove last char
    else
    {
        QString text = input->text();
        input->setText(text.remove(text.length()-1, 1));
    }

    if(history.size() >= 2){ history.pop();}
}

void MainWindow::on_buttonRoot_released()
{
    // TODO nth root
}

/* Called when a user pushes the '(' button or uses the
 * keyboard shortcut SHIFT+9. Appends an opening parenthesis
 * to the input. Disables entry of closing parentheses
 * and operators directly afterwards.
 */
void MainWindow::on_buttonOpenParenth_released()
{
    if(history.top().openParenthAllowed)
    {
        input->setText(input->text().append("("));
        history.push(State(true, false, true, false,
                           history.top().numOpenParenths+1,
                           history.top().numClosingParenths));
    }
}

/* Called when a user pushes the ')' button or uses the
 * keyboard shortcut SHIFT+0. Appends a closing parenthesis
 * to the input. Disables entry of opening parentheses and
 * digits directly afterwards.
 */
void MainWindow::on_buttonCloseParenth_released()
{
    State currentState = history.top();
    if(currentState.closingParenthAllowed &&
            (currentState.numClosingParenths < currentState.numOpenParenths))
    {
        input->setText(input->text().append(")"));
        history.push(State(false, true, false, true,
                           history.top().numOpenParenths,
                           history.top().numClosingParenths+1));
    }
}

void MainWindow::on_buttonXSquared_released()
{
    // TODO x squared
}

void MainWindow::on_buttonSqrt_released()
{
    // TODO square root
}

When the user would like to undo a previous action, they press the Back button, which in turn pops the top of the stack, effectively reverting the calculator to its prior state.

Question

My worry is that this approach may be inefficient, given how many variables a State has and how long a user's input could potentially be. Granted, most of the State members are only bytes, but there are also two ints each.

If this approach of using a Stack of States is indeed inefficient, is there a better solution I could use?

One idea that comes to mind is having each of the slot methods (e.g. on_digit_released() and on_binary_button_released()) check what character came before and proceeding accordingly. I think this would eliminate some of the memory workload, but it would also be a bit messier. Would the trade-off be worth it?

\$\endgroup\$
  • \$\begingroup\$ Also, I would really appreciate if I not be downvoted into oblivion. I've had very bad experiences with Stack Overflow, so I hope I don't run into any of the same problems here. I put a lot of effort into documenting my problem clearly to make it easier for others to understand. My code works. I don't believe I've violated any rules of the site. If I have, please do not close the question—clarify what I need to do to improve the question, and I will edit it accordingly. Thank you. \$\endgroup\$ – AleksandrH Jun 17 '18 at 14:44
  • \$\begingroup\$ Okay, in that case, I'll edit the question to remove the link. \$\endgroup\$ – AleksandrH Jun 17 '18 at 15:20
  • \$\begingroup\$ I tried my best to explain how the code functions and included the relevant piece of information (the struct I'm using to accomplish my goal). If I include anything more than this, you'll be looking at 300+ lines. Every time a button is pressed, a new State is added to the stack. If my explanation wasn't clear, please let me know. \$\endgroup\$ – AleksandrH Jun 17 '18 at 16:00
  • \$\begingroup\$ Alright, going to add it in. \$\endgroup\$ – AleksandrH Jun 17 '18 at 16:06
  • \$\begingroup\$ @bruglesco I've updated the mainwindow .cpp code with some clearer documentation. Would you recommend I edit the original question and add this code? It won't change the essence of the problem, but some of the code will be different. I didn't want to do this before asking in case that's bad practice on the site. Thanks! \$\endgroup\$ – AleksandrH Jun 17 '18 at 17:36
5
\$\begingroup\$

Disclaimer: I'm not familiar with Qt

With that in mind I still have some points I can mention.


My worry is that this approach may be inefficient, given how many variables a State has and how long a user's input could potentially be. Granted, most of the State members are only bytes, but there are also two ints each. If this approach of using a Stack of States is indeed inefficient, is there a better solution I could use?

I agree that this doesn't seem like the best approach. I personally would keep one copy of the State of the calculator and adjust as needed. Want to revert to old State? Simply read the elements of your Stack as you remove them. More on this later.


Ui::MainWindow *ui;
QLabel *input;

It is standard in C++ to put the identifier with the type, like you did here:

Ui::MainWindow* getUI() const { return ui; }

Which also reminds me to point out that consistency leads to increased readability.


connect(ui->button0, SIGNAL(released()), this, SLOT(on_digit_released()));
connect(ui->button1, SIGNAL(released()), this, SLOT(on_digit_released()));
connect(ui->button2, SIGNAL(released()), this, SLOT(on_digit_released()));
connect(ui->button3, SIGNAL(released()), this, SLOT(on_digit_released()));
connect(ui->button4, SIGNAL(released()), this, SLOT(on_digit_released()));
connect(ui->button5, SIGNAL(released()), this, SLOT(on_digit_released()));
connect(ui->button6, SIGNAL(released()), this, SLOT(on_digit_released()));
connect(ui->button7, SIGNAL(released()), this, SLOT(on_digit_released()));
connect(ui->button8, SIGNAL(released()), this, SLOT(on_digit_released()));
connect(ui->button9, SIGNAL(released()), this, SLOT(on_digit_released()));

Code repetition like this that is highly repetitive and sequential is just begging for a loop. Store your buttons in a container and iteration over them.

prefer the range-for loop syntax when you can

for (auto&& button : container_of_buttons)
{
    //...
}

Rule of 3/5/0

You declare a (needed) destructor for cleanup of all your pointers. If you declare a destructor you need a Copy Constructor, a Copy Assignment Operator, a Move Constructor and a Move Assignment Operator as well.


Now back to your design. You could keep 1 instance of State within the calculator class. Then keep a stack of Inputs. Something like:

struct Input {
    bool isOperator;
    int val;
    OperatorType operator;
}

and

enum class OperatorType {
    plus,
    minus,
    et al...
}

When the user provides input then you update the State accordingly and push an Input to stack<Input>. When going back you peek at the top element of your stack and invert the logic accordingly and update your State, then pop off of your stack.


Prefer prefix over postfix

\$\endgroup\$
  • \$\begingroup\$ Thank you for such a thorough review! I'm going to implement most of this. But I'm wondering if your solution of stack<Input> is more efficient than stack<State>. Basically, in my version, for each character in the input, there will be one new State on the top of the stack. So aren't these essentially both scaling linearly, except with your struct having fewer member variables? \$\endgroup\$ – AleksandrH Jun 18 '18 at 10:19
  • \$\begingroup\$ Raw pointers are in fact idiomatic for referring to Qt objects (which are, in general, owned by a parent QObject), so not problematic like they are in other cases. That said, it's certainly not normal to expose a widget's ui object like that! \$\endgroup\$ – Toby Speight Jun 18 '18 at 11:33
  • \$\begingroup\$ @TobySpeight Ah, so is it bad that I retrieved a pointer to the input label? I did this mainly because it's used a lot throughout the code, and I wanted to avoid repetition. \$\endgroup\$ – AleksandrH Jun 18 '18 at 12:30
  • \$\begingroup\$ I meant the (public) getUI() method - which doesn't seem to be used anywhere, so probably best deleted. Let me know if you need me to expand my comment to be an answer. \$\endgroup\$ – Toby Speight Jun 18 '18 at 12:33
  • \$\begingroup\$ @TobySpeight Thank you for letting me know. Ill update my answer accordingly. \$\endgroup\$ – bruglesco Jun 18 '18 at 12:36

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.