TransWikia.com

Library Management System OOP with c++( Part2 of a series )

Code Review Asked by theProgrammer on December 10, 2020

I don’t know if this is acceptable, but I would love to thank the community for their advice concerning my previous post on this project

This is a beginner’s project.

Library management system aims to handle the basic housekeeping of a functional library, so far I have implemented the BookItem class and with advice from the community, also implemented a Date class( not with full funtionalities )

Librarian class though not completed yet is also functional…
I used a list to store the books in the library. I saw some post where vectors are suggested as the goto data structure, I feel list best suit this case, If vectors would be better I would appreciate if you highlight the reasons.

Note am a beginner and have no knowledge about C++ advanced topics yet.

Here is the code

Date.hh

#ifndef DATE_HH
#define DATE_HH
/*****************************************************************
 * Name: Date.hh
 * Author: Samuel Oseh
 * Purpose: Date class method-function prototype
 * ***************************************************************/
#include <iostream>
class Date {
    friend std::ostream &operator<<( std::ostream &, const Date & );
     private:
        /* data-member */
        unsigned int month;
        unsigned int day;
        unsigned int year;

        // utility function
        unsigned int checkDay( int ) const;

    public:
        static const unsigned int monthsPerYear = 12;

        // ctors
        Date() : Date( 1, 1, 1970 ){}
        Date( int m, int d, int y ) { setDate( m, d, y );}
        Date( int m ) : Date( m, 1, 1970 ){}
        Date( int m, int d ) : Date( m, d, 1970 ) {}
        
        // copy operations
        Date( const Date &d ) { *this = std::move(d); }
        Date &operator=( const Date &d ) { month = d.month; day = d.day; year = d.year; return *this; }

        /* method-functions */
        void setDate( int m, int d, int y );
        void setMonth( int m );
        void setDay( int d );
        void setYear( int y );
        unsigned int getMonth() const;
        unsigned int getDay() const;
        unsigned int getYear() const;       
        void nextDay();

        // dtor
        ~Date(){};
};

#endif

Date.cc

/*****************************************************************
 * Name: Date.cc
 * Author: Samuel Oseh
 * Purpose: Date class method-function definitions
 * ***************************************************************/
#include <iostream>
#include <stdexcept>
#include <array>
#include "Date.hh"

void Date::setDate( int m, int d, int y) {
    setMonth( m );
    setDay( d );
    setYear( y );
}

void Date::setDay( int d ) {
    day = checkDay( d );
}

void Date::setMonth( int m ) {
   if ( m >= 1 && m < 13 )
        month = m;
    else
      throw std::invalid_argument( "Month must be between 1-12" );
}

void Date::setYear( int y ) {
    if ( y >= 1970 )
        year = y;
    else
        throw std::invalid_argument( "year must be greater than 1969" );
}

void Date::nextDay() {
    day += 1;
    try {
        checkDay( day );
    } catch ( std::invalid_argument &e ) {
        month += 1;
        day = 1;
    }
    if ( month % 12 == 0 ) {
        year += 1;
        month = 1;
    }
}

std::ostream &operator<<( std::ostream &os, const Date &d ) {
    os << d.month << "/" << d.day << "/" << d.year << " ";

    return os;
} 

// utility function
unsigned int Date::checkDay( int testDay ) const {
    static const std::array < int, monthsPerYear + 1 > daysPerMonth = { 0,31,28,31,30,31,30,31,31,30,32,30,31};

    if ( testDay > 0 && testDay <= daysPerMonth[ month ] )
        return testDay;
    
    if ( month == 2 && testDay == 29 && ( year % 400 == 0 || ( year % 4 == 0 && year % 100 != 0 ) ) )
        return testDay;
    throw std::invalid_argument( "Invalid day for current month and year" );
}

BookItem.hh

#ifndef BOOKITEM_HH
#define BOOKITEM_HH
/*****************************************************************
 * Name: BookItem.hh
 * Author: Samuel Oseh
 * Purpose: BookItem class method-function prototype
 * ***************************************************************/
#include <string>
#include "Date.hh"

enum class BookStatus : unsigned { RESERVED, AVAILABLE, UNAVAILABLE, REFERENCE, LOANED, NONE };
enum class BookType : unsigned { HARDCOVER, MAGAZINE, NEWSLETTER, AUDIO, JOURNAL, SOFTCOPY };

class BookItem {
    private:
        /* data-members */
        std::string title;
        std::string author;
        std::string category;
        Date pubDate;
        std::string isbn;
        BookStatus status;
        BookType type;
    public:
        // ctor
        BookItem() = default;
        BookItem( const std::string &title, const std::string &author, const std::string &cat, const Date &pubDate, 
                const std::string &isbn, const BookStatus status, const BookType type ); 

        // copy operations
        const BookItem& operator=( const BookItem &bookItem );
        BookItem( const BookItem &bookItem ) { *this = std::move(bookItem); }

        /* method-functions */
        void setStatus( BookStatus s ) { status = s; };
        void setType( BookType t ) { type = t;};
        std::string getStatus() const;
        std::string getType() const;
        std::string getTitle() const { return title; }
        std::string getAuthor() const { return author; }
        Date &getPubDate() { return pubDate; }
        void printPubDate() const { std::cout << pubDate; }
        std::string getIsbn() const { return isbn; }
        void setCategory( const std::string &c ) { category = c; }
        std::string getCategory() const { return category; };

        // dtor
        ~BookItem(){}
};
#endif

BookItem.cc

/*****************************************************************
 * Name: BookItem.cc
 * Author: Samuel Oseh
 * Purpose: BookItem class method-function definitions
 * ***************************************************************/
 #include <iostream>
#include "BookItem.hh"

BookItem::BookItem( const std::string &t, const std::string &a, const std::string &c, const Date &d, 
                const std::string &i, const BookStatus s, const BookType ty ) {
                    title = t, author = a, category = c, pubDate = d, isbn = i;
                    setStatus( s );
                    setType( ty );
}

const BookItem &BookItem::operator=( const BookItem &bookItem ) {
    title = bookItem.title;
    author = bookItem.author;
    category = bookItem.category;
    pubDate =  bookItem.pubDate;
    isbn = bookItem.isbn;
    status = bookItem.status;
    type = bookItem.type;

    return *this;
}

std::string BookItem::getStatus() const { 
    if ( status == BookStatus::AVAILABLE )
        return "AVAILABLE";
    else if ( status == BookStatus::REFERENCE )
        return "REFERENCE";
    else if ( status == BookStatus::UNAVAILABLE )
        return "UNAVAILABLE";
    else if ( status == BookStatus::LOANED )
        return "LOANED";
    else if ( status == BookStatus::RESERVED )
        return "RESERVED";
    else
        return "NONE";
} 

std::string BookItem::getType() const {
    if ( type == BookType::AUDIO )
        return "AUDIO";
    if ( type == BookType::HARDCOVER )
        return "HARDCOVER";
    if ( type == BookType::JOURNAL )
        return "JOURNAL";
    if ( type == BookType::MAGAZINE )
        return "MAGAZINE";
    if ( type == BookType::NEWSLETTER )
        return "NEWSLETTER";
    if ( type == BookType::SOFTCOPY )
        return "SOFTCOPY";
    else 
        return "NONE";
}

Librarian.hh

#ifndef LIBRARIAN_HH
#define LIBRARIAN_HH
/*****************************************************************
 * Name: Librarian.hh
 * Author: Samuel Oseh
 * Purpose: Librarian class method-function prototype
 * ***************************************************************/
#include <iostream>
#include <string>
#include "BookItem.hh"
#include <list>

class Librarian {
    private:
        /* data-member */
        std::string name;
        Date dateOfHire;
        std::list<BookItem> *books = new std::list<BookItem>;
    public:
        // ctor
        Librarian() = default;
        Librarian( const std::string &name, const Date &dateOfHire );

        /* basic method-functions */
        void setName( const std::string &name );
        void setDateOfHire( const Date &date );
        std::string getName() const { return name; };
        Date &getDateOfHire() { return dateOfHire; }
        void printDateOfHire() const { std::cout << dateOfHire; }

        /* core functionalities */
        void addBook( const BookItem &book );
        void auditLibrary() const;
        
        // dtor
        ~Librarian(){}
};

#endif

Librarian.cc

/*****************************************************************
 * Name: Librarian.cc
 * Author: Samuel Oseh
 * Purpose: Librarian class method-function definitions
 * ***************************************************************/

#include <iostream>
#include "Librarian.hh"

Librarian::Librarian( const std::string &n, const Date &d ) {
    name = n;
    dateOfHire = d;
}

void Librarian::setName( const std::string &n ) {
    name = n;
}

void Librarian::setDateOfHire( const Date &d) {
    dateOfHire = d;
}

void Librarian::addBook( const BookItem &book ) { 
    if ( books->empty() ) {
        books->push_front( book );
        return;
    }
    for ( auto bk = books->begin(); bk != books->end(); ++bk ) {
        if( book.getTitle() <= bk->getTitle() ) {
            books->insert(bk, book);
            return;
        }
    }
    books->push_back( book );
}

void Librarian::auditLibrary() const {
    std::cout << "Librarian: " << name << ", Date of hire: " << dateOfHire;
    std::cout << "nnBooks:";
    for ( auto bk = books->begin(); bk != books->end(); ++bk ) {
        std::cout << "nName of book: " << bk->getTitle();
        std::cout << "nAuthor of book: " << bk->getAuthor();
        std::cout << "nBook category: " << bk->getCategory();
        std::cout << "nPublication date: ";
        bk->printPubDate();
        std::cout << "nISBN number: " << bk->getIsbn();
        std::cout << "nStatus of book: " << bk->getStatus();
        std::cout << "nType of book: " << bk->getType();
        std::cout << "nn"; 
    }
}
c++object oriented

2 Answers

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]) || (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.

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.

Correct answer by Rish on December 10, 2020

unsigned int month;
unsigned int day;
unsigned int year;

may also be written in one line

unsigned int month, day, year;

You use unsigned int, that's ok, but you should nevertheless use int, especially for small numbers. unsigned int is mostly used for transfering data from and to storage devices / network streams. signed int is better portable, because some other programming languages doesn't support unsigned int. signed int allows often easier debugging, because a -1 is often easier to detect than a 4294967295.

std::string BookItem::getStatus() const { 
    if ( status == BookStatus::AVAILABLE )
        return "AVAILABLE";
    else if ( status == BookStatus::REFERENCE )
        return "REFERENCE";
    else if ( status == BookStatus::UNAVAILABLE )
        return "UNAVAILABLE";
    else if ( status == BookStatus::LOANED )
        return "LOANED";
    else if ( status == BookStatus::RESERVED )
        return "RESERVED";
    else
        return "NONE";
}

This may also be written as:

std::string BookItem::getStatus() const { 
    if ( status == BookStatus::AVAILABLE ) return "AVAILABLE";
    if ( status == BookStatus::REFERENCE ) return "REFERENCE";
    if ( status == BookStatus::UNAVAILABLE ) return "UNAVAILABLE";
    if ( status == BookStatus::LOANED ) return "LOANED";
    if ( status == BookStatus::RESERVED ) return "RESERVED";
    return "NONE";
}

You may also just use a switch/case.

.

When using:

std::cout << "nAuthor of book: " << bk->getAuthor();
std::cout << "nBook category: " << bk->getCategory();

Better do it this way:

std::cout << "Author of book: " << bk->getAuthor() << std::endl;
std::cout << "Book category: " << bk->getCategory() << std::endl;

Otherwise your code looks good at the first view. Maybe add some additional header text description.

Answered by paladin on December 10, 2020

Add your own answers!

Ask a Question

Get help from others!

Recent Questions

Recent Answers

© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP