Event tracking system

Question

I am learning C++ programming and just learned about basic OOP and decided to create a simple project to test my understanding and practice what I've learned. The idea I came up with is an event tracking system where you add events into a calendar and then you get all of your events displayed. I have 2 classes: Event, where your events are created, and Calendar, which holds a vector of all Events. Could you please review my code saying what are the most efficient ways of doing things and the best practices to be followed?

---

Main.cpp

#include "Calendar.h"

int main() {
    Calendar calendar {};

    calendar.add_event("Exam", "urgent", "10/12/2020", "10:30");
    calendar.add_event("Meeting", "non-urgent", "08/11/2020", ("12:20"));

    calendar.display_events();
}

Event.h

#include <string>

class Event {
private:
    std::string event_type;
    std::string event_priority;
    std::string event_date;
    std::string event_time;

public:
    Event(std::string eventType, std::string eventPriority, std::string eventDate,
          std::string eventTime);

    bool display_event() const;

    ~Event();
};

Event.cpp

#include "Event.h"
#include <iostream>
#include <utility>

Event::Event(std::string eventType, std::string eventPriority, std::string eventDate,
             std::string eventTime) : event_type(std::move(eventType)), event_priority(std::move(eventPriority)),
                                             event_date(std::move(eventDate)), event_time(std::move(eventTime)) {
}

bool Event::display_event() const {
    std::cout << "You have " << event_type << " on " << event_date << " at " << event_time << " it's " << event_priority << "\n";
    return true;
}


Event::~Event() = default;

Calendar.h

#include "Event.h"
#include <vector>

class Calendar {
private:
    std::vector<Event> calendar;

public:
    bool display_events() const;

    bool add_event(std::string event_type, std::string event_priority, std::string event_date, std::string event_time);

    const std::vector<Event> &getCalendar() const;

    bool is_event_valid(const std::string& event_date, const std::string& event_time);

    ~Calendar();
};

Calendar.cpp

#include "Calendar.h"
#include <iostream>
#include <utility>

const std::vector<Event> &Calendar::getCalendar() const {
    return calendar;
}

bool Calendar::display_events() const {
    if (!getCalendar().empty()) {
        for (const auto &event : calendar) {
            event.display_event();
        }
        return true;
    } else {
        std::cout << "Your calendar is empty \n";
        return false;
    }
}

bool Calendar::add_event(std::string event_type, std::string event_priority, std::string event_date,
                         std::string event_time) {

    if (is_event_valid(event_date, event_time))
    {
        Event event {std::move(event_type), std::move(event_priority), std::move(event_date), std::move(event_time)};
        calendar.push_back(event);
        return true;
    } else {
        std::cout << "Event is not valid\n";
        return false;
    }
}

bool Calendar::is_event_valid(const std::string& event_date, const std::string& event_time) {
    int day{}, month{}, year{}, hours{}, minutes{};

    day = std::stoi(event_date.substr(0,2));
    month = std::stoi(event_date.substr(3, 2));
    year = std::stoi(event_date.substr(6, 4));
    hours = std::stoi(event_time.substr(0, 2));
    minutes = std::stoi(event_time.substr(3, 2));

    bool is_date_valid = (day > 0 && day <= 24) && (month > 0 && month <= 12) && (year >= 2020 && year <= 3030);
    bool is_time_valid = (hours >= 0 && hours <= 24) && (minutes >= 0 && minutes <= 60);

    if (is_date_valid && is_time_valid) {
        return true;
    } else {
        std::cout << "The event's time or date is not valid\n";
        return false;
    }
}

Calendar::~Calendar() = default;

I am also thinking about adding a feature where you can sort the events by date.

Answer 1

General Observations

Welcome to the Code Review Site. Nice starting question, very good for a begining C++ programmer and a new member of the Code Review Community.

The functions follow the Single Responsibility Principle (SRP) which is excellent. The classes also follow the SRP which is very good as well. You aren't making a fairly common beginner mistake by using the using namespace std; statement. Good use of const in many of the functions.

The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

The SRP is the S in the SOLID programming principles below.

The object oriented design needs some work, for instance the Event class should have an is_valid() method to let each event validate itself, this would be useful when creating a new event. The Calendar class could use this method and doesn't need to know about the private members of the event class. Including access to the private members of the Event class in the Calendar class prevents this from being a SOLID object oriented design.

In object-oriented computer programming, SOLID is a mnemonic acronym for five design principles intended to make software designs more understandable, flexible and maintainable.

Include Guards

In C++ as well as the C programming language the code import mechanism #include FILE actually copies the code into a temporary file generated by the compiler. Unlike some other modern languages C++ (and C) will include a file multiple times. To prevent this programmers use include guards which can have 2 forms:

1. the more portable form is to embed the code in a pair of pre-processor statements

   #ifndef SYMBOL
   #define SYMBOL
   // All other necessary code
   #endif // SYMBOL

2. A popular form that is supported by most but not all C++ compilers is to put #pragma once at the top of the header file.

Using one of the 2 methods above to prevent the contents of a file from being included multiple times is a best practice for C++ programming. This can improve compile times if the file is included multiple times, it can also prevent compiler errors and linker errors.

Class Declarations in Header Files

For both the Event and Calendar classes you define a destructor for the object in the class declaration and then set that destructor in the .cpp file, it would be better to do this in the class declarations themselves. For simple or single line functions such as display_event() you should also include the body of the function to allow the optimizing compiler to decide if the function should be inlined or not.

In C++ the section of the class immediately following class CLASSNAME { is private by default so the keyword private isn't necessary where you have it in your code. Current conventions in object oriented programming are to put public methods and variables first, followed by protected methods and variables with private methods and variables last. This convention came about because you may not be the only one working on a project and someone else may need to be able to quickly find the public interfaces for a class.

Example of Event class refactored

#include <iostream>    
#include <string>

class Event {
public:
    Event(std::string eventType, std::string eventPriority, std::string eventDate,
        std::string eventTime);

    bool display_event() const {
        std::cout << "You have " << event_type << " on " << event_date << " at " << event_time << " it's " << event_priority << "\n";
        return true;
    }

    ~Event() = default;

private:
    std::string event_type;
    std::string event_priority;
    std::string event_date;
    std::string event_time;

};

Object Oriented Design

The Calendar class has dependencies on the private fields of the Event class, the problem with this is it limits the expansion of the code of both classes and makes it difficult to reuse the code which is a primary function of object oriented code. It also makes the code more difficult to maintain. Each class should be responsible for a particular function / job.

You mention sorting the events by date as a possible expansion of the program, in this case you need to add an <= operator to decide what order the events should be in, that operator should be in the Event class, but it looks like you would implement it in the Calendar class.

The following code does not belong in a Calendar class method, it belongs in an Event class method:

    day = std::stoi(event_date.substr(0, 2));
    month = std::stoi(event_date.substr(3, 2));
    year = std::stoi(event_date.substr(6, 4));
    hours = std::stoi(event_time.substr(0, 2));
    minutes = std::stoi(event_time.substr(3, 2));

Currently the only way to create a new event is to try to add it to the calendar, it would be better to create each event on it's own, check the validity of the even and then call the add_event() method in the calendar.

Answer 2

To add to the answers of Aryan Parekh and pacmaninbw, which I agree with:

Avoid repeating the name of a class in its member variables

For example, in class Event, all the member variable names are prefixed with event_, but that is redundant. I would just remove that prefix.

Avoid using std::string unless something is really text

Apart from date/time information, event_priority is something that probably also should not be a std::string, but rather something the C++ language can more easily work with, like an enum class:

class Event {
public:
    enum class Priority {
        LOW,
        NORMAL,
        URGENT,
        ...
    };

private
    std::string type; // this really look like text
    Priority priority;
    ...
};

Using this type consistenly, you should then be able to write something like:

Calendar calendar;
calendar.add_event("Exam", Event::Priority::URGENT, ...);

An enum is stored as an integer, so it is very compact and efficient. There is also no longer a possibility of accidentily adding an invalid or misspelled priority name, like "ugrent". Of course, now you have to add some functions to convert Prioritys to/from human readable text, so it is a bit more work on your part.

Pass Events directly to member functions of Calendar

Instead of having to pass four parameters to add_event(), just pass a single parameter with type Event. This simplifies the implementation of add_event(), and will make it future-proof. Consider for example adding ten more member variables to Event, this way you avoid adding ten more parameters to add_event() as well! Of course, be sure to pass the parameter as a const reference:

class Calendar {
    ...
    bool add_event(const Event &event);
    ...
};

And then you can use it like so:

Calendar calendar;
calendar.add_event({"Exam", Event::Priority::URGENT, ...});

Move is_event_valid() to Event

Checking whether an Event is valid is the responsibility of Event. However, instead of having a (static) member function is_valid() in class Event, consider checking for valid parameters in its constructor, and have it throw a std::runtime_error if it cannot construct it. That way, Calendar::add_event() doesn't have to do any checking anymore: if you managed to pass it an Event, it can only be a valid one at that point.

The caller of add_event() has to handle the possibility of the constructor of class Event throwing, but it already had to handle add_event() returning an error anyway, so it is not much more work.

Answer 3

Looking at your program, I would say that you have done a pretty good job considering that you are just a beginner.

---

private or public?

Let's have a look at your Event class

class Event {
private:
    std::string event_type;
    std::string event_priority;
    std::string event_date;
    std::string event_time;

public:
    Event(std::string eventType, std::string eventPriority, std::string eventDate,
          std::string eventTime);

    bool display_event() const;

    ~Event();
};

The 4 main parts of your event, which are the type, priority, date, and time are declared private.
The problem with this is that now once you have set the Event, there is no way the user can modify it. What if the user's boss decides to re-schedule a meeting, he realizes that he would have to create an entirely new event, and delete the previous one. This wouldn't be as convenient as simply being able to change any attribute of Event.

Another scenario, you have mentioned in your question

I am also thinking about adding a feature where you can sort the events by date.

This means that your Calendar should be able to see the events' dates. But the way you have designed your class, this would be impossible.

For those reasons, I believe making the 4 main attributes of Event public would make sense.

---

Don't encode date/time as std::string.

day = std::stoi(event_date.substr(0,2));
month = std::stoi(event_date.substr(3, 2));
year = std::stoi(event_date.substr(6, 4));
hours = std::stoi(event_time.substr(0, 2));
minutes = std::stoi(event_time.substr(3, 2));

All numbers here are known as magic numbers.
The problem with encoding date/time as std::string is that now if you want to extract any information, you will have to do

std::stoi(event_time.substr(3, 2));

I suggest that you create your own date/time class or even use one already defined in the std::chrono library ( C++ 20).

Here is a very very simple date class

struct Date
{
    int day;
    int month;
    int year;
    
    date(int day, int month, int year)
        : day(day),month(month),year(year)
    {}
};

Now instead of having to do substr again and again which can get weird. You can easily access specific parts of a date

Date date(25,9,2020)
// boss says push the meeting 

date.day = 30;

Note that this is just an example. You also need to validate the date. An example of a nice feature might be post_pone() which can extend a date by a certain number of days.

The sample applies for time, a simple struct will let you get more control of things and also simplify stuff at the same time.

---

Extend Event

Your Event misses a few attributes.

• Place of the event
• Description of the event
• Duration

---

If-else logic

bool Calendar::add_event(std::string event_type, std::string event_priority, std::string event_date,
                         std::string event_time) {

    if (is_event_valid(event_date, event_time))
    {
        Event event {std::move(event_type), std::move(event_priority), std::move(event_date), std::move(event_time)};
        calendar.push_back(event);
        return true;
    } else {
        std::cout << "Event is not valid\n";
        return false;
    }
}

If you simply reverse the conditions, you can simplify the code and remove one branch

bool Calendar::add_event(std::string event_type, std::string event_priority, std::string event_date,
                         std::string event_time) {

    if (!is_event_valid(event_date, event_time))
    {
        std::cout << "Event is not valid\n";
        return false;
    } 
    Event event {std::move(event_type), std::move(event_priority), std::move(event_date), std::move(event_time)};
    calendar.push_back(event);
    return true;
}

• Furthermore, I would suggest that you display the event after saying Event is not valid. Because if you added more than 5-6 events to your calendar, and all you say was Event is not valid, you would have to do a search to track down which event

---

Extend Calendar

Your Calendar class misses some key functions.

• Ability to delete/remove an event
• Modify an event