3
\$\begingroup\$

This is a small program to implement the FVP algorithm (outlined here). I'm still quite new to C++ and don't have a strong grasp of basically any concepts. Concepts I tried to use in this program:

  • OOD
  • Header files
  • Lambda functions
  • std::list and std::vector

I would be grateful for any suggestions on code style, any bugs you see, and any other advice you might have.

main.cpp

#include "fvpalgorithm.h"

int main() {
    FVPAlgorithm algorithm;
    algorithm.run();
}

fvpalgorithm.h

#pragma once

#include <string> // Need strings for Task.
#include <vector> // Need to define an object of type vector here, so must include in the .h.
#include <list>

struct Task {  // Need to give Task struct a body in header.
    std::string taskName;
};

class FVPAlgorithm {

private:
    std::list<Task> longList; // List of Tasks
    std::vector<std::list<Task>::iterator> shortList; // Vector of iterators to tasks in longList

    void addTasks();
    void selectTasks(std::list<Task>::iterator startIterator);
    void promptToDo(std::list<Task>::iterator task);
    //std::list<Task>::iterator compareTasks(std::list<Task>::iterator startIterator);

public:
    void run();
    void printAllTasks();
    void printShortList();
};

fvpalgorithm.cpp

#include "fvpalgorithm.h"

#include <iostream>

/*
----- The algorithm -----

Create a longlist of all tasks.

Add the first task to the shortlist.

Iterate through each task - ask if user would rather do that
        than the last task on the shortlist (Which is the first task in the list, in this case)

If user says no, go to next task.

If user says yes, add task to shortlist.

Continue until no tasks left on longlist.

Tell user to complete last task added to shortlist.

When user has completed last task added to shortlist, remove it from the longlist 
        and begin iterating through longlist again from the index below the task that was just removed.

Ask if the user wants to do it more than the last task on the shortlist.

If the user decides they want to do the last item on the longlist, then just tell them to do the next task
        on the shortlist after they finish it (since there are no more tasks on the longlist that they
        didn't already turn down in favour of the second-last item on the shortlist.

Allow for items being added to end of list.

-------------------------
*/

void FVPAlgorithm::addTasks() {
    std::cout << "Please add task names. Enter q to quit adding tasks." << std::endl;

    std::string taskInput = "";

    while (taskInput != "q") {
        std::getline(std::cin, taskInput);
        if (taskInput != "q") {
            longList.push_back(Task{ taskInput });
            std::cout << "Added task." << std::endl;
        }
    }

    std::cout << "\nFinished adding tasks. The following tasks were added:" << std::endl;
    printAllTasks();
}

void FVPAlgorithm::printAllTasks() {
    for (std::list<Task>::iterator it = longList.begin(); it != longList.end(); ++it) {
        std::cout << it->taskName << std::endl;
    }
}

void FVPAlgorithm::printShortList() {
    for (std::vector<std::list<Task>::iterator>::iterator it = shortList.begin(); it != shortList.end(); ++it) {
        std::cout << (*it)->taskName << std::endl;
    }
}

void FVPAlgorithm::selectTasks(std::list<Task>::iterator startIterator) {

    auto compareTasks = [this](std::list<Task>::iterator it) {
        std::string shortlistedTaskName = shortList.back()->taskName;
        char userChoice = NULL;
        for (it; it != longList.end(); ++it) {
            std::cout << "Would you like to do " << it->taskName << " more than " << shortlistedTaskName << "? (Y/N)" << std::endl;
            std::cin >> userChoice;
            while (true) {
                if (userChoice == 'Y' || userChoice == 'y') { // User wants to do this task more than the current leader.
                    shortList.push_back(it); // Add this task to the end of the shortlist.
                    return it; // Returns the task we stopped on.
                }
                else if (userChoice == 'N' || userChoice == 'n') { break; } // User doesn't want to, move on.
                else std::cout << "Please enter Y or N." << std::endl; break;
            }
            userChoice = NULL;
        }
        return it;
    };

    std::list<Task>::iterator latestTaskChecked = compareTasks(std::next(startIterator, 1)); // longList.begin() is the first element of the vector, and then increments by 1, for second element.
    while (latestTaskChecked != longList.end()) { // If we didn't go through all of the tasks the first time,
        latestTaskChecked = compareTasks(++latestTaskChecked); // Start comparing again from the next task after the one we stopped at.
    }
}

void FVPAlgorithm::promptToDo(std::list<Task>::iterator task) {
    // Instruct user to do the given task.
    std::cout << "You should do " << task->taskName << ". Enter anything when done." << std::endl;
    std::string doneTask;
    std::cin >> doneTask;
    std::cout << "Good job!" << std::endl;
}

void FVPAlgorithm::run() {

    // Add tasks to the longlist.
    addTasks();

    // Begin algorithm loop.
    while (!longList.empty()) { // While we still have tasks left to do,
        if (shortList.empty()) { // If we have nothing on the shortlist,
            shortList.push_back(longList.begin()); // Add the first task to the shortlist
            selectTasks(shortList.back()); // Add any more tasks the user would like, after the last item in shortList.
            promptToDo(shortList.back());
        }
        if (&*shortList.back() != &longList.back()) { // If last item in shortlist isn't last item in longist,
            std::list<Task>::iterator lastCompletedTask = shortList.back(); // Make note of the task we just finished,
            shortList.pop_back(); // and delete it from the shortlist.
            selectTasks(lastCompletedTask); // Compare everything after last task we just finished.
            longList.erase(lastCompletedTask); // Delete the last completed task.
            promptToDo(shortList.back());
        }
        else { // The last item in the shortlist is the last item in the longlist,  
            longList.pop_back(); // so pop them both off,
            shortList.pop_back();
            promptToDo(shortList.back()); // and prompt to do next-last task.
        }
    }
    std::cout << "No tasks remaining!" << std::endl;
}
```
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Couple of small things:

Use emplace_back rather than push_back when you just have the parameters for the constructors:

longList.push_back(Task{ taskInput });

// This is better written as:

longList.emplace_back(taskInput);

The difference between the two:

  • push_back(Task{ taskInput });.
    This creates a "Task" object as an input parameter. It then calls push_back(). If the Task type object is movable (it is) then it is moved into the list otherwise it is copied into the list.

  • emplace_back(taskInput);
    This creates an object in place in the list. This means the Task object in the list is created at the point and place it is needed without needing to copy anything.

The emplace_back() is preferred (but only very slightly). This is because if the object being put in the container is not movable then it will be copied (copies can be expensive). So it is preferred to create the object in place.

Now. Since the paramer 'taskInput' is never going to be used again we could also use std::move() to move the string to the constructor so potentially avoiding a copy of the string.

longList.emplace_back(std::move(taskInput));

Prefer the range based for for looping over containers:

for (std::list<Task>::iterator it = longList.begin(); it != longList.end(); ++it) {
    std::cout << it->taskName << std::endl;
}

Can be simplified to:

for (auto const& task: longList) {
    std::cout << task.taskName << "\n";
}

So what is happening here?
The range based for works with any object that can be used with std::begin(obj) and std::end(obj). These methods by default simply call the begin/end method on obj.

So:

for (auto const& item: cont) {
    // CODE
}

Can be considered as shorthand for:

{
    auto end = std::end(cont);
    for (auto iter = std::begin(cont); iter != end; ++iter) {
        auto const& item = *iter;

        // CODE
    }
}

Prefer to use "\n" rather than std::endl.

The difference here is that std::endl flushes the stream (after adding the '\n') character. It is usually ill advised to manually flush stream (unless you have done the testing). This is because humans are bad at deciding when a stream needs to be flushed and the code will flush the stream if it needs to be flushed automatically.

One of the biggest complaints from beginners about C++ is that std::cout is not as fast as printing to stdcout in C. The main culprit of this is usually down to inappropriate flushing of the std::cout buffer. Once that is fixed the speed of these streams are nearly identical.


Don't copy strings if you just need a refeence:

   std::string shortlistedTaskName = shortList.back()->taskName;

This copies the string into shortlistedTaskName. If you just need a short reference to the value use a reference.

   std::string& shortlistedTaskName = shortList.back()->taskName;
      //     ^^^   This is a reference to the object on the right.

    for (it; it != longList.end(); ++it) {
         ^^   Does nothing.

    // write like this.
    for (; it != longList.end(); ++it) {

Don't use NULL. This is old school C for a null pointer. Unfortunately it is actually the number 0 and can thus accidentally be assigned to numeric types. Which is confusing as they are not pointers.

In C++ we use nullptr to refer to the null pointer. It can only be assigned to pointer objects and thus is type safe.


Don't use NULL to represent nothing.

    char userChoice = NULL;

That is not a concept in C++. Here userChoice is a variable. It exists and will always have a value. The trouble is that char is a numeric type so assigning NULL too userChouce gave it the integer value of 0 which is the same as the char value '\0'.

You can leave it unassigned or put a default value it in. In this context since you are about to read into it I would just leave it unassigned.

    char userChoice;

As long as you write into it before reading its value everything is OK.


Reading from a stream can fail.

std::cin >> userChoice;

Reading a stream can fail. Even the std::cin input can get an EOF signal which means nothing more can be read.

So always check the result of the read.

if (std::cin >> userChoice) {
    // Something was successfully read into the character.
}

I don't see why you need this loop.

        while (true) {
            if (userChoice == 'Y' || userChoice == 'y') {
                return it;
            }
            else if (userChoice == 'N' || userChoice == 'n') {
                break;
            }
            else std::cout << "Please enter Y or N." << std::endl;
            break;
        }

You could simplify this to:

        if (userChoice == 'Y' || userChoice == 'y') {
            return it;
        }
        else if (userChoice != 'N' && userChoice != 'n') {
            std::cout << "Please enter Y or N." << "\n"
        }

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for the advice! I'll make some changes you recommended. If nobody else submits an answer, or yours is the best, I'll accept yours :) Do you have any suggestions re: lambda functions? How is the actual structure of the code - is anything redundant or weird? \$\endgroup\$ Apr 3, 2020 at 8:48
2
\$\begingroup\$

As per signature main function must return int value

#include "fvpalgorithm.h"

int main() {
    FVPAlgorithm algorithm;
    algorithm.run();
}

Task Structure: Why is taskName public? Here taskName looks like task identifier. Will this identifier change post creation? Do you intend to support case where task is created with name X and then later changed to Y? Better to take name at construction time and then provide a task name getter and avoid setter.

If you are aware that task is going to have more attributes apart from name, then defining a separate task struct makes sense. But if it is only going to have name, and you want to keep your code readable make TaskName an alias for std::string (using/typedef) ad get rid of struct completely.

struct Task {  // Need to give Task struct a body in header.
    std::string taskName;
};

Class FVPAlgorithm: This class violates SRP. It has three responsibilities. Read data from cin, Execute FVP algorithm, and Print. So apart from changes in algorithm steps, his class will also have to change if task input method changes (Say rather than cin, you start reading from file, or start consuming task list returned by some other module), or tasks have to be printed to streams other than cout. Also because this class is taking care of input and output, it must take case errors that can happen during input/output. Summary: Remove input/output responsibility from this class and let it focus on algorithm implementation and its error cases.

class FVPAlgorithm { 
private:
    std::list<Task> longList; // List of Tasks <nkvns>: tasks is better name for this variable
    std::vector<std::list<Task>::iterator> shortList; // Vector of iterators to tasks in longList <nkvns>: selectedTasks is better name for this variable. As per current name, shortList is actually a vector not list.

    void addTasks();
    void selectTasks(std::list<Task>::iterator startIterator);
    void promptToDo(std::list<Task>::iterator task);
    //std::list<Task>::iterator compareTasks(std::list<Task>::iterator startIterator);

public:
    void run();
<nkvns>: Print* method should be marked const. Print can't change state of the object.
    void printAllTasks(); 
    void printShortList();
};

Sanitize input before consumption: Check/sanitize input before accepting it. User can input garbage string of arbitrary length. You can go out of memory.

if (taskInput != "q") {
            longList.push_back(Task{ taskInput });
            std::cout << "Added task." << std::endl;
        }
    }

Use auto to improve readability: auto is good way of improving readability. Here you can use auto for iterators like std::list<Task>::iterator. Also loop in print* method is read loops so use cbegin and cend.

void FVPAlgorithm::printAllTasks() {    
    for (std::list<Task>::iterator it = longList.begin(); it != longList.end(); ++it) {
        std::cout << it->taskName << std::endl;
    }
}

void FVPAlgorithm::printShortList() { for (std::vector::iterator>::iterator it = shortList.begin(); it != shortList.end(); ++it) { std::cout << (*it)->taskName << std::endl; } }

CompareTasks lambda: It is good use of lambda. But given that CompareTasks has many lines of codes, Define TaskComparator separately. You may choose to make it part of Task class itself. A class can provide method for comparison. If you want to have dynamic comparison criteria (Based on task in selection or some other user criteria) use strategy pattern to decide on comparison strategy at run time.

auto compareTasks = [this](std::list<Task>::iterator it) {
        std::string shortlistedTaskName = shortList.back()->taskName;
        char userChoice = NULL;
        for (it; it != longList.end(); ++it) {
            std::cout << "Would you like to do " << it->taskName << " more than " << shortlistedTaskName << "? (Y/N)" << std::endl;
            std::cin >> userChoice;
            while (true) {
                if (userChoice == 'Y' || userChoice == 'y') { // User wants to do this task more than the current leader.
                    shortList.push_back(it); // Add this task to the end of the shortlist.
                    return it; // Returns the task we stopped on.
                }
                else if (userChoice == 'N' || userChoice == 'n') { break; } // User doesn't want to, move on.
                else std::cout << "Please enter Y or N." << std::endl; break;
            }
            userChoice = NULL;
        }
        return it;
    };
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Welcome to Code Review. Commenting inline makes this rather confusing. \$\endgroup\$
    – chicks
    Apr 4, 2020 at 17:24
  • \$\begingroup\$ Updated based on comment from @chicks \$\endgroup\$
    – nkvns
    Apr 5, 2020 at 7:03
  • \$\begingroup\$ Much improved. Thank you. \$\endgroup\$
    – chicks
    Apr 5, 2020 at 14:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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