Online book reader system

Question

I am learning to design classes, and in the effort to improve, I have designed an online book reader system with the following requirements in mind:

1. Allow person to create and renew membership
2. person should be able to search database of books
3. person should be able to check out max 1 book
4. should be move pages forward and backward
5. should be able to read only 1 book

I am sure there are several loopholes, but please suggest how I can improve on these aspects along with others that I am unaware of:

1. The design seems to be missing the essence of a controller.
2. The class ReaderSystem is acting as an interface and is doing lot of heavy lifting.
3. Is a state machine warranted with these requirements?
4. With a class like Search, which I wanted to separate out with rationale that search mechanism should be independent, would doing single functionality still warrant a separate class?

Note: In order to not bloat code too much, I have skipped some of the places with getters/setters.

book.h

#ifndef ONLINE_BOOK_PERSON_H
#define ONLINE_BOOK_PERSON_H
#define BOOK_NAME 20
#define MAX_PAGES 1000
namespace READER
{
 class Book
 {
 public:
 Book(const char* name, int pages);
 ~Book();
 char* getBookName();
 int getNumPage();
 int* getCurrentPage();
 bool moveForwardPage();
 bool moveBackPage();
 bool setAvailability(bool val);
 bool getAvailability() const;
 private:
 char book_name[BOOK_NAME];
 int total_pages[MAX_PAGES];
 int *pcurrent_page;
 bool isavailable;
 };
}
#endif

person.h

#ifndef ONLINE_PERSON_H
#define ONLINE_PERSON_H
#define NAMELENGTH 40
namespace READER
{
 class Person
 {
 public:
 Person (const char* first, const char* last);
 Person();
 ~Person();
 bool setFirstName(const char* first_name);
 char* getFirstName()const;
 bool setAccountNumber(int account_number); // skipping getters from here on
 bool isMember() const;
 int memValidTill() const;
 private:
 char firstname[NAMELENGTH];
 char lastname[NAMELENGTH];
 int account_number;
 bool current_member;
 int membership_begin; // should keep time struct - for simiplicity
 int valid_till; // shortcut
 };
}
#endif

library.h

#ifndef ONLINE_LIBRARY_H
#define ONLINE_LIBRARY_H
#define NAMELENGTH 40
#define MAX_BOOKS
namespace READER
{
 class Library
 {
 public:
 Library(int maxbooks);
 ~Library();
 int getTotalBooks() const;
 int totalCheckoutBooks() const;
 Book* checkout(char *name);
 bool checkIn(Book *pbook);
 bool isBookAvailable(Book *pbook);
 private:
 Book book_catalog[MAX_BOOKS];
 int total_books;
 int total_checkout_books;
 };
}
#endif

membership.h

#ifndef ONLINE_BOOK_MEMBERSHIP_H
#define ONLINE_BOOK_MEMBERSHIP_H
#define MAX_MEMBERS 100
#include "person.h"
namespace READER
{
 class Membership
 {
 public:
 Membership(Person *person);
 ~Membership();
 bool isMember(Person *Person); // for simplicity have kept entire person
 bool isMember(int account_number);
 bool createMember(Person *Person);
 bool extendMembership(Person *Person);
 bool extendMembership(int account_number);
 int membershipValidTill(int account_number); //skipping option of Person
 private:
 Person memberslist[MAX_MEMBERS];
 };
}
#endif

booktrans.h

#ifndef ONLINE_TRANSAC_H
#define ONLINE_TRANSAC_H
#include "book.h"
#include "booksearch.h"
#include "library.h"
namespace READER
{
 class BookTrans
 {
 public:
 BookTrans();
 bool checkOut(Book *pbook);
 bool checkIn(Book *pbook);
 private:
 Book *pbook;
 BookSearch *psearch;
 Library *pLibrary;
 };
}
#endif

booksearch.h

#ifndef ONLINE_BOOK_SEARCH_H
#define ONLINE_BOOK_SEARCH_H
#include "library.h"
#include "book.h"
namespace READER
{
 class BookSearch
 {
 public:
 BookSearch(Library *plibrary);
 Book* searchBook(Book *pbook);
 private:
 Library *plibrary;
 Book *pbook;
 };
}
#endif

readersystem.h

#ifndef ONLINE_BOOK_READER_SYSTEM_H
#define ONLINE_BOOK_READER_SYSTEM_H
/*
 * This is main interface - through which end user can
 * interact and get the tasks done
 * */
namespace READER
{
 class ReaderSystem
 {
 public:
 ReaderSystem(Library *plibrary);
 bool createMember(Person *pperson);
 bool validMembership(Person *pperson);
 int memberValidTill(Person *pperson);
 bool renewMember(Person *pperson);
 bool isBookAvailable(Book *pbook);
 bool checkOutBook(Book *pbook);
 bool checkInBook(Book *pbook);
 private:
 Book *pbook;
 Library *plibrary;
 Person *pperson;
 BookSearch *pbooksearch;
 Membership *pmembership;
 BookTrans *pbooktrans;
 };
}
#endif

• user1118321: Can you add some code showing how you expect someone to use these classes? It's not clear to me what you expect from someone calling these. Would they create a Library, fill it with Books, then pass it to the ReaderSystem? How is the BookSearch class intended to be used? Also, where's the code? This is just the interface.

Answer 1

Design

Well your code seems to do a lot more than your initial design.
So first of I would simplify all the code to do what the design is not do everything that you can imagine that the system needs to do in the future. Keep it simple.

Code Review.

Stop using pointers. You should practically never use a pointer in C++ code (unless you are doing something low level). You are working at a high level so there should be no need to do the low level stuff.

Rather than arrays of characters refer to use std::string. Arrays can overflow and you need to plant specific code to make sure that does not happen. The std::string on the other hand will dynamically grow to be the correct size so you don't need to think about checking the size.

Also an array converts into a pointer way to easily and looses all concept of size which makes it hard to check the size of the array.

Comments on specific things.

Don't use #define or other macros.

#define BOOK_NAME 20

This is a C thing. C++ we have better ways of handling all situations were macros appear. In this case use a const (or in C++11 contexpr).

constexpr int bookName = 20;

As mention above don't use pointers and prefer std::string.

 Book(const char* name, int pages);
 // It should do this:
 Book(std::string const& name, int pages);

Using getters/setters is bad design.

 bool setAccountNumber(int account_number); // skipping getters fro here on

Thye expose implementation details beyond the boundry of the class. Usually what happens is you use a bunch of getters to get info the do some opertion then use a setter to update the object with new state. This is the wrong way to think about it. You should make operation a member of the class and ask the objec to mutate its own state by calling opertion member method.

Stop doing this:

 char firstname[NAMELENGTH];
 // Much easier to use string
 std::string firstName;

Why does a librry have a max?

 Library(int maxbooks);

Why are you returning a pointer to the book?

 Book* checkout(char *name);

Am I supposed to delete the book when I am finished with it. Can the result every be Null? Pointers cause all sorts of problems about ownership. Returning a reference is nearly always the better solution.

Are there object of type "BookSearch"?

 class BookSearch

I suppose there can be. But I don't think you are at that level of sophistication yet (not saying that in a bad way). Would this not just be a method on the Library.

• oneday: Thank u so much for the review and comments. Let me work on them. I guess you can see I am carrying habits of C :). With regard to one of the points .. "Using getters/setters is bad design" - For this comment can you please post a snippet for one of the case how would you have approached it or point me to some link which shows me reference to understand this point better ?
• Loki Astari: @oneday cplusplus.com/forum/lounge/101305
• oneday: Thank u really appreciate the link and your noble effort in helping people like me learn the art !!!
• Loki Astari: @oneday: Thanks for the tick. But I really think Edward did a much better job with his answer.

Answer 2

I see a number of things that may help you improve your code. First I will list the more major design issues and then a number of smaller items.

Reconsider your design

One good way to think about designing an object oriented system is to look at your text description of the problem. Nouns become objects and verbs become functions. In your description, the nouns are "person", "membership", "database of books" (which you've quite reasonably already shortened to Library), "book" and "pages".

I think we can already see that a Book might be a collection of Pages and that a Library might be a collection of Books. That leaves "person" and "membership". For this, I'd be inclined to say that it's actually the Library which checks and tracks "membership". For that reason, there should probably be Library member functions which enforce library rules such as checking membership and assuring that each person only has 0 or 1 book checked out.

What isn't clear from the specification is if only 1 person at a time may read a particular book. That's how it works with physical books, of course, but perhaps not necessarily with online books. I'll assume that we can allow books to be checked out by multiple people simultaneously, but that there is perhaps a limit to that number.

You already have a Person, Book, Library class. I'd be inclined to scrap the rest and just concentrate on those, until it becomes necessary to introduce another class.

Using such a scheme, the main interface would be with the Library. For example, one might have member functions to add a Book or add a Person, then functions for checking out and checking in a Book which would create a BookTransaction type temporarily associating an individual Book with an individual Person as described in more detail in a later suggestion.

Use standard collections

The current Library class currently has a fixed array of Book objects. Instead, it would make more sense to have a dynamic collection such as std::vector. This would allow for easier management of books and would eliminate the need for member function total_books. Similarly, your Membership class is a collection of Person objects. I would subsume that into the Library class instead and use a std::vector.

Don't use raw pointers

In very many places, raw pointers are used either as parameters or as return values. This is generally best avoided. It's usually better to use reference unless it really makes sense to have an uninitialized pointer (possibly nullptr) in that place. For example, the BookSearch class has this:

class BookSearch
{
public:
 BookSearch(Library *plibrary);
 Book* searchBook(Book *pbook);
private:
 Library *plibrary;
 Book *pbook;
};

Since no destructor is supplied, the plibrary and pbook objects are not deleted by the destruction of the BookSearch object, and therefore the BookSearch class does not own them. This is an unsafe design because some other part of the program could easily alter or destroy the pointed-to object and the BookSearch object would then have an invalid pointer. Better would be to use std::shared_ptr for shared objects, or make duplicates where it makes sense to do so.

Prefer const to #define

The Book class currently has these two lines:

#define BOOK_NAME 20
#define MAX_PAGES 1000

There are a couple of problems with this. First, they probably don't have any meaning outside the class, but they are global in scope. Second, they do not have any associated type which can lead to errors. For both of these reasons, I'd recommend instead making those const or constexpr values and putting them within the Book class. Even better, however, would be to eliminate them entirely, since a std::string should be used instead of a char *, and the pages could be a std::vector of Page objects, eliminating the need for an arbitrary cap on the size of either.

Think carefully about object attributes

Certainly some things, such as the title and the number of pages are rightly attributes (member data) of a Book object, but what about current page? It seems to me that that would actually more likely to be a function of the reader. For instance, if two people are simultaneously reading a single book (if that's allowed by your system), then the current page number would be more accurately expressed as either member data of a Person object or possibly instead a class expressly for the individual association between a Book and a Person. The latter would be useful in case we someday wish to allow, as most libraries do, checking out more than one book at a time. That might be a better use for your BookTransaction class which would then be a unique association between a Person and a Book. The Library would then maintain a collection of such BookTransaction objects, creating them as books are checked out, and deleting them when books are checked back in.

Use const where practical

A number of member function are marked const but there are certainly more that could be. For example, your existing Membership::isMember() probably won't need to modify the Membership object, so it should be const.

Don't allow access to internal class data

The Person class has the following data member:

char firstname[NAMELENGTH];

and also this public member function:

char* getFirstName()const;

We can't know because you don't show the implementation of this function, but you really must not simply pass back a pointer to internal data. Better would be to make a copy and return that. If, as previously recommended, you use std::string objects, this quite simple.

Make your destructors virtual

If you are creating a class hierarchy that may be reused or extended (and it's more often the case than not), your object destructors should be declared virtual to safely allow their use. If you're absolutely sure that this will never be the case, then you should use the final specifier to enforce this.

Don't use "Hungarian notation"

Using "Hungarian notation" is unwise for a number of reasons; the primary one being that it makes your code harder to read. Even if there were an advantage to using it, it's not used consistently in this code, so it's worth very little in the current context and should be eliminated.

• oneday: Thank u so much for the review and suggestions. Also could you please suggest me route to make a controller class which acts as central activity and how to approach it
• Edward: I've added it to my answer as part of the first section.
• oneday: Got it . I forgot to mention I loved the way you explained to approach about noun with objects. With regard to Library being the main interface, you are suggesting to remove some components/functionality from reader system and put it in library ? . So way I am being advised on approaching the designing is - having a central "controller" interface so that each object doesnt deal directly with other object. is your recommendation of making class library such controller with change suggested by you ?