Library Management System con OOP [chiuso]

Question

Sto creando un sistema di gestione delle biblioteche. Attualmente ho finito di creare la classe Book e BookItem. Puoi indicare qualsiasi cosa che ritieni non necessaria, inefficiente o una cattiva pratica generale in modo che io possa imparare da essa e crescere nella mia carriera di programmatore

Book.hh

#ifndef BOOK_HH
#define BOOK_HH
/*****************************************************************
 * Name: Book.hh
 * Author: Samuel Oseh
 * Purpose: Book class method-function prototype
 * ***************************************************************/

#include <string>
class Book {
    public:
        /* method function */
        Book()  = default;
        ~Book(){}
        virtual std::string getStatus() const = 0;
        virtual std::string getType() const = 0;
};

#endif

BookItem.hh

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

class BookItem : Book{
    private:
        /* data */
        std::string name;
        std::string author;
        std::string pubDate;
        std::string isbn;
        std::string status;
        std::string type;
        bool onlineCopy{false};
        /* utility function */
        bool validatePubDate( std::string date ) const;
        
    public:
        BookItem() = default;
        BookItem( const std::string &name, const std::string author, const std::string pubDate, \
                const std::string isbn, const std::string status, const std::string type, bool onlineCopy = false ); 
        BookItem( const BookItem &bookItem ) { *this = std::move(bookItem); }
        BookItem& operator=( const BookItem &bookItem );
        void setStatus( std::string status );
        void setType( std::string status );
        std::string getStatus() const { return status; };
        std::string getType() const { return type; }
        void setOnlineCopy() { onlineCopy = true; }
        bool hasOnlineCopy() { return onlineCopy; }
        std::string giveName() const { return name; }
        std::string giveAuthor() const { return author; }
        std::string givePubDate() const { return pubDate; }
        std::string giveIsbn() const { return isbn; }
        
        static const std::string *const statusPtr;
        static const std::string *const typePtr;
        
        ~BookItem(){}

};

#endif

BookItem.cc

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

std::string statusCopy[5] = { "RESERVED", "AVAILABLE", "UNAVAILABLE", "REFERENCE", "LOANED" };
std::string typeCopy[] = {"ART", "BASIC MEDICAL SCIENCES", "LAW", "PROGRAMMING", "COMPUTER SCIENCE", "NURSING", "PHARMARCY", "MAGAZINE",\
                            "ARTICLE", "JOURNAL", "BANKING", "NEWSLETTER"};
const std::string *const BookItem::typePtr = typeCopy;
const std::string *const BookItem::statusPtr = statusCopy;

BookItem:: BookItem( const std::string &nme, const std::string author, const std::string pubDate, \
                const std::string isbn, const std::string status, const std::string type, bool onlineCopy ) : Book() {
                    this->name = name;
                    this->author = author;
                    if ( validatePubDate( pubDate ) )
                        this->pubDate = pubDate;
                    else 
                        throw std::invalid_argument("Invalid publication date");
                    this->isbn = isbn;
                    setStatus( status );
                    setType( type );
                    this->onlineCopy = onlineCopy;
}


BookItem &BookItem::operator=( const BookItem &bookItem ) {
    name = bookItem.name;
    author = bookItem.author;
    pubDate = bookItem.pubDate;
    isbn = bookItem.pubDate;
    status = bookItem.status;
    type = bookItem.type;
    onlineCopy = bookItem.onlineCopy;
    return *this;
}

bool BookItem::validatePubDate( std::string date ) const { 
    struct tm tm;
    if ( strptime( date.c_str(), "%d/%m/%y", &tm ) ) 
        return true;
    return false;
}

void BookItem::setStatus( std::string status ) {
    for ( const std::string *p = begin(statusCopy); p != end(statusCopy); ++p ) {
        if ( *p == status ) {
            this->status = status;
            return;
        }
    }
    throw std::invalid_argument("Invalid Status");
}

void BookItem::setType( std::string type ) {
    for ( const std::string *p = begin(typeCopy); p != end(typeCopy); ++p ) {
        if ( *p == type ) {
            this->type = type;
            return;
        }
    }
    throw std::invalid_argument("Invalid Type");
}
```

Answer 1

1. Il primo problema è che non vedo perché lo fai tramite OOP. Perché Bookè un'interfaccia? Ha quasi zero funzionalità. Usa l'interfaccia per oggetti con funzionalità complesse quando hai diverse possibili implementazioni. Mentre la classe Book può servire solo per ciò downcastingche di per sé è meglio evitare se non necessario. Inoltre ha un bug che il distruttore non è virtuale: deve essere virtuale quando lavori con classi astratte / polimorfismo.

2. Lanciare un costruttore è una cattiva idea in generale e specialmente nei casi OOP. Se le classi non fossero completamente costruite, un distruttore potrebbe incasinare e corrompere il codice che porta agli UB, quindi per lanciare un programmatore costruttore deve assicurarsi che tali problemi non si verifichino e ci sono alcuni casi complessi in cui non è banale . Dovresti evitarlo memorizzando la data non come una stringa ma come una classe / struttura dedicata. E lascia che l'utente chiami la conversione dalla stringa quando necessario. Se la conversione fallisce, il lancio avverrà al di fuori del costruttore.

3. BookItem& operator=( const BookItem &bookItem );perché implementarlo? Basta impostarlo come predefinito o non scrivere affatto. E dichiarare il distruttore ~BookItem(){}è inutile. Inoltre ti manca l'assegnazione del movimento e il costruttore di mosse.

4. Dovresti usare enum classper BookTypee BookStatus. Non ho idea del motivo per cui usi una stringa: è solo uno spreco e una confusione per gli utenti. Basta aggiungere una stringa generale a / da conversioni enum.

5. std::string giveName() const ...e simili hanno nomi strani. Cosa giveNamesignifica? Non lo capisco Dovrebbe essere lo getNamestesso per gli altri giveXXXsostituirlo con getXXX. Inoltre, i libri non hanno nomi: hanno titoli. Quindi sostituisci "nome" con "titolo".

6. Le funzioni std::string giveAuthor()e simili restituiscono std::stringche potenzialmente rendono un'allocazione e allocazioni non necessarie sono lente e causano la frammentazione della memoria a lungo termine. Puoi evitarlo restituendo il riferimento const const std::string&o std::string_view.

7. void setOnlineCopy()... inoltre non segue le convenzioni di denominazione. Le persone usano setper fornire l'interfaccia per impostare il valore a una determinata proprietà come, void setX(int x){mX = x;}ma qui devi solo impostare il valore booleano su true. Dichiara la funzione come void setOnlineCopyStatus(bool bHasOnlineCopy).