Simple Qt GUI to list user-created categories and tasks

Question

I've written a very simple program using Qt that I intend to continue working on as I continue learning C++ and Qt.

I'm mostly wondering about my general code and comment style, specifically in terms of C++, less so about the actual program (which is boring and featureless at the moment).

I've uploaded the full code to my GitHub here: https://github.com/cmkluza/task_organizer

The main file of interest is mainwindow.cpp:

#include "mainwindow.h"
#include "ui_mainwindow.h"
#include <QInputDialog>
#include <iostream>
MainWindow::MainWindow(QWidget *parent) :
 QMainWindow(parent),
 ui(new Ui::MainWindow) {
 ui->setupUi(this);
 // disable "add tasks" unless a category is selected
 ui->addTaskButton->setEnabled(false);
 // at startup, display that there's no categories
 noCatItem = std::make_unique<QTreeWidgetItem>();
 noCatItem->setIcon(0, QIcon(arrow));
 noCatItem->setText(0, "No Catagories");
 ui->catTreeWidget->addTopLevelItem(noCatItem.get());
}
MainWindow::~MainWindow() {
 delete ui;
}
/**
 * @brief MainWindow::on_addCatButton_clicked Inserts a new category at the end of the current list of categories.
 */
void MainWindow::on_addCatButton_clicked() {
 unselectCategories();
 bool ok;
 auto catName = QInputDialog::getText(this, "Add Category", "Category Name:",
 QLineEdit::Normal, QString(), &ok);
 if (ok && !catName.isEmpty()) {
 auto *newItem = new QTreeWidgetItem();
 // if "no categories" is still there, remove it and make this the "active" category
 if (curIsNoCat()) {
 ui->catTreeWidget->takeTopLevelItem(0);
 newItem->setIcon(0, QIcon(arrow));
 }
 newItem->setText(0, catName);
 ui->catTreeWidget->addTopLevelItem(newItem);
 }
}
/**
 * @brief MainWindow::on_addTaskButton_clicked Inserts a new task at the end of the curren't category's list of tasks.
 */
void MainWindow::on_addTaskButton_clicked() {
 unselectCategories();
 // if there's no current item, do nothing
 if (!ui->catTreeWidget->currentItem()) return;
 bool ok;
 auto taskName = QInputDialog::getText(this, "Add Task", "Task Name:",
 QLineEdit::Normal, QString(), &ok);
 if (ok && !taskName.isEmpty()) {
 auto *cur = ui->catTreeWidget->currentItem();
 auto *newItem = new QTreeWidgetItem(cur);
 newItem->setText(0, taskName);
 cur->addChild(newItem);
 }
}
/**
 * @brief MainWindow::on_nextButton_clicked Makes the next category active.
 * The "next" category is the subsequently displayed one.
 */
void MainWindow::on_nextButton_clicked() {
 unselectCategories();
 // make the now previous category "inactive"
 ui->catTreeWidget->topLevelItem(curCatIndex)->setIcon(0, QIcon());
 // get the next category index
 if (++curCatIndex >= ui->catTreeWidget->topLevelItemCount())
 curCatIndex = 0; // wrap
 // make the new category "active"
 ui->catTreeWidget->topLevelItem(curCatIndex)->setIcon(0, QIcon(arrow));
}
/**
 * @brief MainWindow::unselectCategories Deselcts all selected categories, which also makes "add task' un-clickable.
 */
void MainWindow::unselectCategories() {
 // can only unselect if there's items selected
 if (!ui->catTreeWidget->selectedItems().isEmpty()) {
 ui->catTreeWidget->clearSelection();
 // disable add tasks
 ui->addTaskButton->setEnabled(false);
 }
}
/**
 * @brief MainWindow::on_catTreeWidget_itemSelectionChanged Adjusts whether or not "add task" is enabled based on what's selected/
 * "Add task" should only be enabled when the user has a single, valid category or task under which the new task can be added.
 */
void MainWindow::on_catTreeWidget_itemSelectionChanged() {
 if (curIsNoCat()) return;
 // if a top-level category is selected, enable the add tasks button
 for (auto &item : ui->catTreeWidget->selectedItems()) {
 if (!item->parent()) {
 ui->addTaskButton->setEnabled(true);
 break;
 }
 }
}
/**
 * @brief MainWindow::curIsNoCat Determines whether the current item is the "no category" item.
 * @return true if "no categories" is still in the list.
 */
bool MainWindow::curIsNoCat() {
 return ui->catTreeWidget->indexOfTopLevelItem(noCatItem.get()) != -1;
}

• Toby Speight: It's very hard to review without the definitions of MainWindow and Ui::MainWindow. You should include these directly in the question, rather than behind a link.
• Calak: If your problem is solved do not hesitate to validate the answer or/and to vote it up if it would useful.
• Cameron: Thanks for the heads up, Toby, I'm not overly familiar with Code Review and didn't want to post too many huge blobs of code. I'll keep that in mind going forward!

Answer 1

Include order :

(削除) You completely reversered order of includes. (削除ここまで) You could include standard header first, then 3rd party header and finally yours. Here's the two way and the explanation.

Raw pointer

You use std::unique_ptr for some widget but not for all nor for ui, why? If you do, you don't want to delete ui anymore thanks to RAII.

Use name that makes sense

Try to use full name, eg: curIsNoCat can be renamed to hasNoCategory

Comments/code ratio

When you have too much comment comparing to your code, that is because your code isn't explicit by itself, so what it do is not enought obvious.

• Cameron: Thanks for the suggestions! The include order and naming does deserve some refactoring. I understand inconsistency using raw vs smart pointers, so I'll have to see how to refactor that, as Qt seems more apt to using raw pointers. In terms of comments, do you have any specific suggestions on what I should look at to learn how to better comment? I've seen arguments ranging from "no comments, your code should be explicit" to "comments to declare intent" to "err on the side of too many comments". I haven't spotted any obvious golden ratio.
• Calak: You Can, per example, (1) make a setActiveCategory function, where you set the icon of the given element to make it active. (2) have a removedNoCategories which remove the "No categories" element and return true or false if it was present (and so, removed). (3) in on_addCatButton_clicked you change to if (removedNoCategories()) setActiveCategory (newWidget); and you got rid of comments making code explicit. (It's just a proof of concept, for the purpose of the example)
• Snowhawk: I'll disagree with the ordering suggestion and will say that going local to global avoids latent usage errors. Catch these errors by ensuring that every header file of a component parses by itself, without externally-provided declarations or definitions. Including the prototype/interface for an implementation ensures no critical pieces of info needed for the physical interface of that component is missing. If there is a critical piece missing, you'll find out as soon as you compile. Interface > Project > 3rd Party > System/Standard. Lakos discusses this in "Large Scale C++ Software Design".
• Calak: @Snowhawk you're right, I should not be so extreme about #include ordre. I've modified my response.
• Addy: @Cameron Working with C++'s smart pointers and Qt's raw pointers can be jarring. I generally prefer to stick with the new QWidget syntax when dealing with UI widgets, if just so I don't accidentally add a shared_ptr to some parent widget using get().