3
\$\begingroup\$

I have written a cashflow calculator. Any suggestions on how to improve the code below? Income and expenses have a similar pattern.

#ifndef MAINWINDOW_H
#define MAINWINDOW_H

#include <QMainWindow>
#include <QStringListModel>
#include <QSettings>

namespace Ui {
class MainWindow;
}

class MainWindow : public QMainWindow
{
    Q_OBJECT

public:
    explicit MainWindow(QWidget *parent = nullptr);
    ~MainWindow();

private slots:
    void on_AddIncome_clicked();
    void on_AddExpenses_clicked();

    void on_DeleteIncome_clicked();
    void on_DeleteExpenses_clicked();

    void AppendIncomeList();
    void AppendExpensesList();

    void CalculateTotalIncome();
    void CalculateTotalExpenses();

    void CalculateCashFLow();
    void CashOnCashReturn();

    void LoadSettings();
    void SaveSettings();

    void on_Investment_editingFinished();

private:
    Ui::MainWindow *ui;

    QString                 m_SettingsFile;

    QStringListModel        *m_CashFlowModel;
    QStringListModel        *m_IncomeModel;
    QStringListModel        *m_ExpensesModel;
    QStringListModel        *m_CashOnCashModel;

    QStringList             m_ExpensesList;
    QStringList             m_IncomeList;

    std::vector<double>     m_IncomeVector;
    std::vector<double>     m_ExpensesVector;
};

#endif // MAINWINDOW_H

#include "mainwindow.h"
#include "ui_mainwindow.h"

#include <QDebug>

MainWindow::MainWindow(QWidget *parent) :
    QMainWindow(parent),
    ui(new Ui::MainWindow)
{
    ui->setupUi(this);

    m_SettingsFile = "C:/Temp/config.ini";

    m_IncomeModel = new QStringListModel();
    m_ExpensesModel = new QStringListModel();
    m_CashFlowModel = new QStringListModel();
    m_CashOnCashModel = new QStringListModel();

    ui->IncomeView->setEditTriggers(QAbstractItemView::NoEditTriggers);
    ui->IncomeView->setModel(m_IncomeModel);

    ui->ExpensesView->setEditTriggers(QAbstractItemView::NoEditTriggers);
    ui->ExpensesView->setModel(m_ExpensesModel);

    LoadSettings();

    CalculateTotalIncome();
    CalculateTotalExpenses();
    CalculateCashFLow();
    CashOnCashReturn();

}

MainWindow::~MainWindow()
{
    SaveSettings();


    delete ui;
}

void MainWindow::on_AddIncome_clicked()
{
    AppendIncomeList();
    CalculateTotalIncome();
    CalculateCashFLow();
    CashOnCashReturn();
}

void MainWindow::on_AddExpenses_clicked()
{
    AppendExpensesList();
    CalculateTotalExpenses();
    CalculateCashFLow();
    CashOnCashReturn();
}

void MainWindow::on_DeleteIncome_clicked()
{
    QModelIndex index = ui->IncomeView->currentIndex();
    m_IncomeModel->removeRows(index.row(),1);

    CalculateTotalIncome();

    ui->IncomeView->setModel(m_IncomeModel);

    CalculateCashFLow();
    CashOnCashReturn();
}

void MainWindow::on_DeleteExpenses_clicked()
{
    QModelIndex index = ui->ExpensesView->currentIndex();
    m_ExpensesModel->removeRows(index.row(),1);

   CalculateTotalExpenses();

   ui->ExpensesView->setModel(m_ExpensesModel);

   CalculateCashFLow();
   CashOnCashReturn();
}

void MainWindow::AppendIncomeList()
{
    QString Text(ui->Income->text());
    Text.append(" ").append(QString::number(ui->IncomePrice->value()));

    m_IncomeList << Text;
    m_IncomeModel->setStringList(m_IncomeList);
}

void MainWindow::AppendExpensesList()
{
    QString Text(ui->Expenses->text());
    Text.append(" ").append(QString::number(ui->ExpensesPrice->value()));

    m_ExpensesList << Text;
    m_ExpensesModel->setStringList(m_ExpensesList);
}

void MainWindow::CalculateTotalIncome()
{
    m_IncomeList = m_IncomeModel->stringList();

    m_IncomeVector.clear();

    foreach(QString item, m_IncomeList)
    {
        QStringList items = item.split(" ");
        m_IncomeVector.push_back(items[1].toDouble());
    }

    ui->IncomeTotal->setText(QString::number(std::accumulate(m_IncomeVector.begin(),m_IncomeVector.end(),0.0)));

}

void MainWindow::CalculateTotalExpenses()
{
    m_ExpensesList = m_ExpensesModel->stringList();

    m_ExpensesVector.clear();

    foreach(QString item, m_ExpensesList)
    {
        QStringList items = item.split(" ");
        m_ExpensesVector.push_back(items[1].toDouble());
    }

    ui->ExpensesTotal->setText(QString::number(std::accumulate(m_ExpensesVector.begin(),m_ExpensesVector.end(),0.0)));

}

void MainWindow::CalculateCashFLow()
{
    double CashFlow = ui->IncomeTotal->text().toDouble() - ui->ExpensesTotal->text().toDouble();

    QStringList StringList;
    StringList << "Monthly Cash Flow " << QString::number(CashFlow);
    StringList << "Annual Cash Flow " << QString::number(CashFlow * 12);
    m_CashFlowModel->setStringList(StringList);

    ui->CashFlow->setModel(m_CashFlowModel);
}

void MainWindow::CashOnCashReturn()
{
    QAbstractItemModel      *m_model = ui->CashFlow->model();

    QModelIndex index = m_model->index(1,0);
    double result = (index.data().toDouble() / ui->Investment->value()) * 100.0;

    QStringList list;
    list << "Monthly " << QString::number(result).append("%");

    index = m_model->index(3,0);
    result = (index.data().toDouble() / ui->Investment->value()) * 100.0;

    list << "Annual " << QString::number(result).append("%");

    m_CashOnCashModel->setStringList(list);
    ui->CashOnCash->setModel(m_CashOnCashModel);

}

void MainWindow::LoadSettings()
{
    QSettings Settings(m_SettingsFile, QSettings::IniFormat);

    double IncomePrice = Settings.value("IncomePrice", "").toDouble();
    ui->IncomePrice->setValue(IncomePrice);

    double ExpensesPrice = Settings.value("ExpensesPrice","").toDouble();
    ui->ExpensesPrice->setValue(ExpensesPrice);

    QString IncomeId = Settings.value("IncomeId","").toString();
    ui->Income->setText(IncomeId);

    QString ExpensesId = Settings.value("ExpensesId","").toString();
    ui->Expenses->setText(ExpensesId);

    double InitialInvestment = Settings.value("InitialInvestment","").toDouble();
    ui->Investment->setValue(InitialInvestment);

    QStringList IncomeList = Settings.value("IncomeModel","").toStringList();
    m_IncomeModel->setStringList(IncomeList);

    QStringList ExpensesList = Settings.value("ExpensesModel","").toStringList();
    m_ExpensesModel->setStringList(ExpensesList);

    restoreGeometry(Settings.value("mainWindowGeometry").toByteArray());
    restoreState(Settings.value("mainWindowState").toByteArray());
}

void MainWindow::SaveSettings()
{
    QSettings Settings(m_SettingsFile, QSettings::IniFormat);

    double IncomePrice = ui->IncomePrice->value();
    double ExpensesPrice = ui->ExpensesPrice->value();
    double InitialInvestment = ui->Investment->value();
    QString IncomeId = ui->Income->text();
    QString ExpensesId = ui->Expenses->text();
    QStringList IncomeModel = m_IncomeModel->stringList();
    QStringList ExpensesModel = m_ExpensesModel->stringList();

    Settings.setValue("IncomeModel",IncomeModel);
    Settings.setValue("ExpensesModel",ExpensesModel);
    Settings.setValue("InitialInvestment",InitialInvestment);
    Settings.setValue("IncomeId", IncomeId);
    Settings.setValue("ExpensesId", ExpensesId);
    Settings.setValue("IncomePrice", IncomePrice);
    Settings.setValue("ExpensesPrice", ExpensesPrice);

    Settings.setValue("mainWindowGeometry", saveGeometry());
    Settings.setValue("mainWindowState", saveState());
}

void MainWindow::on_Investment_editingFinished()
{
    CashOnCashReturn();
}
\$\endgroup\$
1
  • \$\begingroup\$ Doesn't compile - "ui_mainwindow.h" is missing. \$\endgroup\$ Oct 1 '19 at 8:05
0
\$\begingroup\$

Here's my suggestions:

  1. This is strange:

    namespace Ui {
    class MainWindow;
    }
    

    it should be like this:

    namespace Ui {
        class MainWindow;
    }
    
  2. In C++, the type is very important, so C++ programmers tend to emphasize the type in declarations. Therefore, the base type of the pointer type is put together with the asterisk — instead of:

    explicit MainWindow(QWidget *parent = nullptr);
    

    it is more common to do:

    explicit MainWindow(QWidget* parent = nullptr);
    
  3. You are holding a raw pointer to Ui::MainWindow:

    Ui::MainWindow *ui;
    

    initializing it with new:

    ui(new Ui::MainWindow)
    

    and then call delete in the destructor:

    delete ui;
    

    This is error prone, and makes your class have undefined behavior when copied. And the memory is leaked if the destructor doesn't get called (i.e., the constructor failed later). The preferred way is to use a unique_ptr: (given that Ui::MainWindow is an incomplete type)

    std::unique_ptr<Ui::MainWindow> ui;
    

    This avoids all problems — your class automatically releases the object on destruction, and the class is not copyable.

    Same for the string list models.

  4. The constructor is exception unsafe — memory is leaked if the news throw. Use std::unique_ptr for all the pointers. However, there's a subtlety here — it seems that you are transferring the ownership of the string list models to the Qt controller. You need to use release here. Like this: (assumes that the pointers are changed to std::unique_ptr)

    MainWindow::MainWindow(QWidget* parent) :
        // same
    {
        // same
    
        // the relevant constructor of std::unique_ptr is explicit
        m_IncomeModel = std::make_unique<QStringListModel>();
        m_ExpensesModel = std::make_unique<QStringListModel>();
        m_CashFlowModel = std::make_unique<QStringListModel>();
        m_CashOnCashModel = std::make_unique<QStringListModel>();
    
        ui->IncomeView->setEditTriggers(QAbstractItemView::NoEditTriggers);
        ui->IncomeView->setModel(m_IncomeModel.release());     // use release
    
        ui->ExpensesView->setEditTriggers(QAbstractItemView::NoEditTriggers);
        ui->ExpensesView->setModel(m_ExpensesModel.release()); // use release
    
        // same
    }
    
  5. Does auto help here?

    QModelIndex index = ui->IncomeView->currentIndex();
    
\$\endgroup\$
3
  • \$\begingroup\$ MainWindow is not copyable, as it inherits from non-copyable QMainWindow, so point 4 is less of a problem. It's still better to use a unique pointer, of course (one usually also has to declare the constructor, so we can define it =default in the implementation, where Ui::MainWindow would be a complete type. \$\endgroup\$ Oct 1 '19 at 8:08
  • \$\begingroup\$ @L.F. Qt has some interesting quirks and features... For Point 2: MainWindow and Ui::MainWindow result from a MainWindow.ui form file being parsed, with Ui::MainWindow containing the autogenerated code and MainWindow being the class/file to hold use-written code, so that is rather common and even sensible. \$\endgroup\$
    – CharonX
    Oct 1 '19 at 8:36
  • \$\begingroup\$ @CharonX Thank you. I have removed the (Note A). \$\endgroup\$
    – L. F.
    Oct 2 '19 at 7:04

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.