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.