4

First of all, I am new here to c++ and I'm trying to learn it. Also new to stackoverflow. Find it quite hard to be honest. If you have additional comments in terms of my code and how i can improve it, please let me know as I am still in the learning process.

ok I was I just creating a online booking system using object oriented programming.

Ok so the main issue is that I dont understand why system.setDisplay(1234); isn't printing anything. I have tried everything and just isn't adding up. OnlineBookingSystem is the class is being used to call setDisplay(id) which then invoked the display class. If you can help it would mean the world to me and the error i`m getting is:

runtime error: member call on null pointer of type 'User' (solution.cpp) SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior prog_joined.cpp:179:54

#include <vector>
#include <string>
#include <iostream>
#include <memory>
#include <queue>
using namespace std;

enum class BookGenre
{
    Horror,Adventure,Romance,Comic
};
class Book
{
    private:
        BookGenre genre;
        string title;
        size_t id;
    public:
        Book(string title,size_t id,BookGenre genre):title(title),id(id),genre(genre){}
        string getTitle(){return title;}
        size_t getId(){return id;}
        BookGenre getGenre(){return genre; }
};
class Library
{
    private:
        vector<shared_ptr<Book>> listOfBooks;
    public:
        Library(){};
        void addBook(string title,size_t id,BookGenre genre)
        {
            listOfBooks.push_back(make_shared<Book>(title,id,genre));
        }
        shared_ptr<Book> getBook(size_t id)
        {
            for(auto&x:listOfBooks)
            {
                if(x->getId()==id)
                {
                    return x;
                }
            }
            return nullptr;
        }
        void removeBook(size_t id)
        {
            for(auto it=listOfBooks.begin();it!=listOfBooks.end();it++)
            {
                if((*it)->getId()==id)
                {
                    listOfBooks.erase(it);
                }
            }
        }
};
class User
{
    protected:
        size_t id;
        string username;
     
    public:
        User(size_t id,string username):id(id),username(username)
        {
 
        }
        virtual ~User(){}
        size_t getId(){return id;}
        string getUsername(){return username;}

};
class Employee:public User{
    private:
        double salary;
    public:
        Employee(size_t id,string username,double salary):User(id,username),salary(salary)
        {
        }
        void setSalary(double salary)
        {
            this->salary=salary;
        }
        double getSalary(){return salary;}
     
};
class Customer:public User{
    private:
           bool membership;
    public:
        Customer(size_t id,string username):User(id,username)
        {
            membership=false;
        }
        void setMemberActive()
        {
            membership=true;
        }
        bool isMemberActive()
        {
            return membership;
        }
};
class UserManager
{
    private:
        vector<shared_ptr<User>>listOfUsers;
        queue<shared_ptr<Customer>>queue;
    public:
        UserManager()
        {

        }
      
        void addCustomer(size_t id,string username)
        {
            listOfUsers.push_back(make_shared<Customer>(id,username));
        }
        void removeCustomer(string username)
        {
            for(auto it=listOfUsers.begin();it!=listOfUsers.end();it++)
            {
                if(dynamic_pointer_cast<Customer>(*it))
                {
                    if((*it)->getUsername()==username)
                    {
                        listOfUsers.erase(it);
                    }
                }
            }
        }
        shared_ptr<Customer> getCustomer(string username)
        {
            for(auto it=listOfUsers.begin();it!=listOfUsers.end();it++)
            {
                if(dynamic_pointer_cast<Customer>(*it))
                {
                    if((*it)->getUsername()==username)
                    {
                        return dynamic_pointer_cast<Customer>(*it);
                    }
                }
            }
            return nullptr;
        }
        void addToQueue(string username)
        {
            queue.push(getCustomer(username));
        }
        void removeCurrentCustomer()
        {
            queue.pop();
        }
        shared_ptr<Customer> getNextCustomer()
        {
            if(queue.empty())
            {
                return nullptr;
            }
            return queue.front();
        }
        /*
            same process for user;
        */
};
class Display
{   
    private:
        shared_ptr<Customer> m_customer;
        shared_ptr<Book> m_book;
    public:
        Display(shared_ptr<Customer> _customer,shared_ptr<Book> _book ):m_customer(_customer),m_book(_book)
        {

        }
        shared_ptr<Customer> getUser(){return m_customer;}
        shared_ptr<Book> getBook(){return m_book;}
        void displayInfo()
        {
            cout<<"Customer username: "<<m_customer->getUsername()<<endl;
            cout<<"Member Active: "<<m_customer->isMemberActive();
            cout<<"book id: "<<m_book->getId()<<endl;
            cout<<"book title: "<< m_book->getTitle()<<endl;
        }

};
class OnlineBookingSystem
{
    private:
        UserManager manager;
        Library library;
        shared_ptr<Display>display;
    public:
        OnlineBookingSystem()
        {
            UserManager manager;
            Library library;
            this->manager=manager;
            this->library=library;
            this->display=nullptr;
        }
        Library getLibrary()
        {
            return library;
        }
        UserManager getUserManager()
        {
            return manager;
        }
        void  setDisplay(size_t id)
        {
            display=make_shared<Display>( manager.getNextCustomer(),library.getBook(id));
            display->displayInfo();
        }
        shared_ptr<Display> getDisplay()
        {
            return this->display;
        }
};
int main()
{

    OnlineBookingSystem system;
    auto lib=system.getLibrary();
    lib.addBook("Adventure of Pablo",1234,BookGenre::Adventure);
    auto manager=system.getUserManager();
    manager.addCustomer(2020,"Michael");
    auto _customer=  manager.getCustomer("Michael");
    _customer->setMemberActive();
    manager.addToQueue("Michael");
    system.setDisplay(1234);

    return 0;
}
  • Unrelated (and doesn't matter a bit here): Regardless of the order used in member initializer list, member variables are initialized in the order they are defined. `genre` is initialized first in `Book` even though it's last in the list. There's no interdependence among the members in `Book`, but when there is, watch out! – user4581301 Sep 23 '20 at 03:46
  • `auto lib=system.getLibrary();` is 100% value semantics. The `Library` is returned by value and stored by value, so `lib.addBook("Adventure of Pablo",1234,BookGenre::Adventure);` operates on a copy. You needed to start testing your code much earlier, I'm afraid. – user4581301 Sep 23 '20 at 03:55
  • @user4581301 Yeah, that ended up being the underlying problem (but with `manager` instead of `lib`). By the way, nice tip about the initialization order; I didn't know that. Then again.. haven't done C++ in a while :) – brads3290 Sep 23 '20 at 03:57
  • thank you so much, I am so happy right now, i felt so down but yes I will follow all of ure tips! – NoobAllStar Sep 23 '20 at 04:02
  • Biggest tip I can give is get an IDE with a good debugger. With a debugger you can step through your program and inspect what it does line by line (and with a good debugger you can do a lot more than that). As soon as you catch the program doing something you didn't expect, usually taking the wrong path or storing the wrong value, you've found a bug. More work to follow understanding and fixing the bug, but finding it is an important early step. – user4581301 Sep 23 '20 at 04:07
  • @brads3290 I'm glad I found that by reading a book. Could have been an absolute pain in the figuring out what had gone wrong if I ran into it as a logic error. g++ will warn you if you have the warning option cranked up. Visual Studio probably has a warning for it, too. I just don't know what level – user4581301 Sep 23 '20 at 04:14
  • manager.getNextCustomer(), library.getBook(id) both reurns the nullptr – Build Succeeded Sep 23 '20 at 05:24

2 Answers2

1

Problems I see:

Problem 1

Since the return type of OnlineBookingSystem::getLibrary() is Library, the line

auto lib=system.getLibrary();

construct lib as a copy of the object in system. Any changes made to lib are changes to the copy, not to the Library object in system.

To fix the problem, change the return type to a reference:

Library& getLibrary()
{
    return library;
}

and capture the return value also as a reference in main.

auto& lib=system.getLibrary();

Problem 2

Similar to Problem 1 but this time it is in OnlineBookingSystem::getUserManager. Change its return type to be a reference:

UserManager& getUserManager()
{
    return manager;
}

and capture the return value also as a reference in main.

auto& manager=system.getUserManager();

Problem 3

Adopt defensive programming at every step until something is a performanced bottleneck. If a return value of a function can be nullptr, check the return value at the point of invocation and deal with the case when the return value is indeed nullptr.

Update OnlineBookingSystem::setDisplay to:

    void  setDisplay(size_t id)
    {
        display=make_shared<Display>( manager.getNextCustomer(),library.getBook(id));
        if ( display )
        {
            display->displayInfo();
        }
        else
        {
           // Deal with the nullptr case
           std::cout << "Unable to find a book with id " << id << std::endl;
        }
    }
R Sahu
  • 204,454
  • 14
  • 159
  • 270
0

The problem
It comes down to your call to system.getUserManager() returning a copy of OnlineBookingSystem's manager field, instead of a pointer to it.

As a result, when you call manager.addToQueue("Michael"), it is adding Michael to the UserManager instance that is local to int main(), instead of the instance that's held in system.

Your function OnlineBookingSystem::setDisplay(size_t) makes a call to manager.getNextCustomer(). Because you've added Michael to a different instance of manager, this returns nullptr.

How to fix it
You simply need to modify OnlineBookingSystem::getUserManager() to return a pointer to manager instead of a copy of it:

UserManager* getUserManager()
{
    return &manager;
}

Then modify the calling code to use the pointer dereference operator (->) instead of a period to access methods on manager.

brads3290
  • 1,926
  • 1
  • 14
  • 20
  • 2
    Consider returning a reference rather than a pointer. – user4581301 Sep 23 '20 at 03:56
  • @user4581301 Yeah you could be right. OP is also using safe pointers so I probably should have done that too anyway.. If you don't mind, is there a practical difference between using a reference and a pointer in this example? The only thing I can think of is the ability to assign to `manager` from the calling code, but given that it's private perhaps that isn't the best option? – brads3290 Sep 23 '20 at 04:02
  • true I could had used reference but as I am using a smart pointer, it is as good no? – NoobAllStar Sep 23 '20 at 04:04
  • @NoobAllStar Actually, as far as I'm aware (and backpedalling what I said about my answer using smart pointers), smart pointers are mostly useful when allocating from the heap, to help prevent memory leaks. I don't think they have much use with pointers to stack-allocated variables. I'd like to hear what user4581301 has to say about it though, he's clearly more experienced than I am. – brads3290 Sep 23 '20 at 04:08
  • 1
    Practical difference between the pointer and the reference, probably not. The compiler may be able to more easily apply a few optimizations to the reference, but that's it. You can do less with the reference, and that provides extra defense against stupid mistakes. Semantically the difference is loaded with context. Most important to me is by returning a reference, readers know that `OnlineBookingSystem` [owns the `Library`](https://stackoverflow.com/questions/49024982/what-is-ownership-of-resources-or-pointers) – user4581301 Sep 23 '20 at 06:20
  • 1
    That said, I question the returning a reference or a pointer to the `Library`. The `Library` is a `private` member, and as soon as you add a `public` accessor with zero access restrictions you have effectively made the `Library` `public`. Anyone can do whatever they want to it, and they can hold it forever, possibly even longer than the lifetime of the `OnlineBookingSystem` it came from. Might be the correct choice here, but I'd think about it extra hard before doing it. The same applies to any setter function. – user4581301 Sep 23 '20 at 06:27
  • 1
    I can't explain my thinking on Smart pointers in a comment. Use Smart pointers to formalize ownership of an object. My general advice is nail down who owns what objects and when. It makes the rest of the decisions much easier to make. Prefer to use the containers to directly hold objects where possible. The container owns the object and pass references around. Use `std::unique_ptr` where the container cannot directly hold the object. Pass references to the object or transfer ownership. Do not use `std::shared_ptr` unless you really do have multiple simultaneous owners of the object. – user4581301 Sep 23 '20 at 06:41
  • @user4581301 Thanks for taking the time to elaborate on that, I appreciate it! This has given me a great new perspective on how to go about using references/pointers. I'd never really thought about managing "ownership" as a concept before. Putting things in those terms certainly helps one decide the appropriate thing to do. Thanks again! – brads3290 Sep 23 '20 at 06:48
  • Useful other reading: [What is meant by Resource Acquisition is Initialization (RAII)?](https://stackoverflow.com/questions/2321511/what-is-meant-by-resource-acquisition-is-initialization-raii). Smart pointers are wonderful examples of RAII. – user4581301 Sep 23 '20 at 06:52
  • thank you so much,learned so much, for learning sake, i tried changed this onlinebookingsystem : unique_ptr manager and unique_ptr getUserManager() { return move(this->manager); } but when i try to call this in main: auto temp=system.getUserManager(); , it keeps returning nullptr which does not make sense to me as I am moving the ownership of uniqueptr. What could be causing this?Again you guys have already helped me so much! – NoobAllStar Sep 24 '20 at 15:09
  • I think i got it, its because i changed the ownership of the pointer, so now the OnlineBookSystem does no longuer own the ownership of that object,so when im trying to access the getmanager in setdisplay, it points to nothing, is this right? – NoobAllStar Sep 24 '20 at 15:16