Skip to main content
improved answer
Source Link
Rish
  • 2.4k
  • 1
  • 9
  • 22
if((testDay <= daysPerMonth[month]) || (isLeapYear() && month == 2 && testDay == 29 && isLeapYear())

Putting isLeapYear() first helps in short circuit evaluation, meaning if isLeapYear() fails, the rest of the conditions won't be checked.

if((testDay <= daysPerMonth[month]) || (month == 2 && testDay == 29 && isLeapYear())
if((testDay <= daysPerMonth[month]) || (isLeapYear() && month == 2 && testDay == 29))

Putting isLeapYear() first helps in short circuit evaluation, meaning if isLeapYear() fails, the rest of the conditions won't be checked.

Source Link
Rish
  • 2.4k
  • 1
  • 9
  • 22

constexpr

Since monthsPerYear is a compile time constant, you should declare it constexpr instead of const. See this answer for more details about constexpr

Date constructors

Your Date constructors take int whereas your data members are unsigned int.

Your constructors are calling another constructor of the same class, which in turn calls setDate method, which just seems like a pointless route. Typically, you would initialize your data members like this:

Date():
     d{1}, m{1}, y{1970}
{
   /// EMPTY
}

Date(unsigned int d, unsigned int m, unsigned int y):
          d{d}, m{m}, y{y}
{
   /// EMPTY
} 

Notice that the call to setDate method is now redundant? While it doesn't matter much for builtin types, it might degrade your performance if you have heavy user-defined types since they will be default constructed. More about member initialization lists

The call to setDate could be replaced by call to a method called validateDate(), whose sole purpose is to validate the date, instead of validating AND setting the values of the data member. A lot of your member functions can be simplified by just using validateDate().

On another note, I question the purpose of the last two user provided types. Ask yourself what's the use case of setting just the day or day/month?

Since your class only contains builtin types, you could omit the body and simply declare it as default.

Date(const Date& rhs) = default;

See here for what default does.

Same goes for the copy assignment operator.

Your use of std::move is pretty much pointless when it comes to builtin types. Anyway, you probably don't want to use std::move in a copy constructor.

checkDay

You're using the value of year before setting it.

I would put the condition to check if it's a leap year inside its own method.

bool isLeapYear() const
{
     return year % 400 == 0 || ( year % 4 == 0 && year % 100 != 0 );
}

Then your checkDay condition can be written more succinctly as

if((testDay <= daysPerMonth[month]) || (month == 2 && testDay == 29 && isLeapYear())

Also, consider returning a bool whether the test succeeds or not, and let the caller of the method decide what to do if it the test fails (such as throw an invalid argument exception).

BookItem

Again, use member initialization lists to set your data members. Use default for the copy constructor and copy assignment operator.

Use switch statements

In your getStatus and getType methods, use switch statements. They lend well to this kind of scenario and look much cleaner.

switch(status)
{
    case BookStatus::AVAILABLE:
        return "AVAILABLE";
    case BookStatus::Reference:
        return "REFERENCE";
    ...
    default:
        return "NONE";
}

Use default for destructor

Since your BookItem destructor isn't doing any non-trivial, you should just declare it default and let the compiler handle it.

~BookItem() = default;

Return const std::string& or std::string_view

Your getter methods return a copy of a std::string, which isn't something you always want (especially for getters). std::string allocates on the heap (well, sometimes it doesn't; look up Small String Optimization if you're interested), which means every time you call a getter, odds are memory is being allocated and deallocated on the heap. If you're not going to change the data, you're just wasting time constructing a new string.

Consider returning a const reference const std::string&, or if you have C++17 compliant compiler, a std::string_view. See here for std::string_view

std::vector over std::list

I can see why you'd want std::list, since you want to insert books in a sorted manner. However, in almost all cases, you want to use std::vector over a std::list. In fact, I would argue that you don't ever need a std::list over std::vector. It's to do with the fact the std::vector stores elements contiguously, which benefits from something called cache locality. That is an answer on its own, so you should see this. Cache friendly code

You can use std::sort on the std::vector, and using a lambda as a custom comparator.

std::vector<BookItem> inventory.
inventory.push_back(...);
inventory.push_back(...);
...

std::sort(inventory.begin(), inventory.end(), [](const BookItem& a, const BookItem& b){ return a.getTitle() < b.getTitle(); });

new keyword

Unless you have good reason, you want to use the STL provided smart pointers instead of new and delete. Right now, your code is leaking memory because you haven't called delete on the list, and this is a biggest pitfall of using raw new and delete.

And why are using allocating the list on the heap at all? std::list allocates on the heap by default; there is no reason to allocator the list object on the heap.