Sign in to follow this  
HiMK

Advice On C++ Class Design & Implementation

Recommended Posts

Hello

 

I am doing a beginner C++ project and its simply a Library Mangement System where a student will hire, give back a book, book and hire records are stored in a binary file etc.

 

I'm approaching the project as though the project would be used by other developers and that I need to design the classes so they cannot be incorrectly used, the project can be scaled/extended at a later date easily and avoid gotchas/bugs down the track so I am being very strict (overkill almost) with my implementation.

 

Therefore, I have some features of the LibraryBook class that I want to implement but dont quite know if this is the best way of doing. If you could provide advice below it would be much appreciated.

 

Classes:

- LibraryManagementSystem (LMS)

- LibraryBook

- Student

 

LibraryBook features I want to implement:

- The class has the expected private properties of title, author, availability, id. I have made the LMS a friend class so that it and only it can change the books availability, maybe even author, title (for scalability/extendability). Would you use friend here or maybe another technique?

- A book object should only ever be created by the LMS, this makes sense I guess because a Student shouldn't be able to make a new book only the LMS. So I was thinking of making the LibraryBook class private but that means a Student wont even be able to hold a book (as a property in their class). I am thinking I should only ever give a handle of the book to the Student? Maybe this is the case for the application of the factory design pattern.

- A LibraryBook must have its title, author, etc. defined always. So I have made the constructor explicit. Should I make the default constructor private? That way a book can only ever be created with defined title, author, etc.

 

LMS features I want to implement:

- I want to store the LibraryBooks in a std::map <string, std::shared_ptr<LibraryBook>>. This is because a student will most likely ask/search for the book by title (title string = key), the LibraryBook dynamic memory will be automatically deleted and I can give the Student a weak_ptr to the LibraryBook which means they cant change anything on that object.

 

Knowing what I want to achieve above; can you provide advice on design patterns and etc. I should use and any errors I have made in my implementation below?

class Student
{
public:
        // I dont think that hire book should be a student method but a LMS method because the LMS issues a book, creates a record etc.
	
	weak_ptr<LibraryBook> curBook; // is this ok as public? or should only the LMS be able to set a students book? Maybe this property only should be friend with LMS?
		
protected:
	
private:
        const string firstName;
        const string lastName;
	const unsigned int ID;
		
};

class LibraryBook
{
public:
	friend class LibraryManagementSystem;
		
	
protected:
	
private:
	explicit LibraryBook(string nTitle, string nAuthor) : title(nTitle), author(nAuthor) { } // will this mean noone not even LMS can create a book using the default constructor? They have to use this one only?
	LibraryBook();
	// No need to implement destructor because implicitly created and theres no dyn memory in this class
	LibraryBook(const LibraryBook& lB); 		// dont implement to disallow copying a book
	void operator=(const LibraryBook& lB); 		// dont implement to disallow copying a book
	
	const string title;
	const string author;
	const unsigned int ID;
		
};

class LibraryManagementSystem
{
public:
	static LibraryManagementSystem& Instance() 
	{
		static LibraryManagementSystem instance;
		return instance;
	}
		
	weak_ptr<LibraryBook> hireBook(string bookTitle, Student& student)
	{
		weak_ptr<LibraryBook> book; // initialised as null ptr I think?
			
		if (libraryBooks.find(bookTitle) == libraryBooks.end())
		    // what should I return? An empty/null weak_ptr?
				
		book = libraryBooks[bookTitle]; // is this cast allowed/possible? Is it allowed to write/overwrite a weak ptr?
		student.curBook = book;
			
		// TODO: create LMS record of who has book, when due back, etc.
			
		return book;
	}
	
protected:
	
private:
	LibraryManagementSystem() {}
	~LibraryManagementSystem() {}
	LibraryManagementSystem(const LibraryManagementSystem& lms);  // dont implement to disallow copy constructor
	void operator=(const LibraryManagementSystem& lms); 			// dont implement assignment operator
		
	map<string, shared_ptr<LibraryBook>> libraryBooks;
};
Edited by HiMK

Share this post


Link to post
Share on other sites

This sounds a lot like homework.

 

Anyways, while you aren't too far off. "Map" does provide one minor problem in the implementation of design. But there are work arounds for it. You will see it later.


I also wouldn't recommend use of smart pointers for this. It's extra brain work you'll have to do in the end. And without an IDE it can be a pain in the arse to debug. And while they are "safer" they are pretty much a hassle for simple tasks.

 

The other bit to think about is what happens when a student wants to check out more than one book? Easy solution is to simply use an array of pointers. Or just an array of indexes. Or an array of strings. What have you.

 

 

If you want to keep student info private, I recommend adding functions who's soul purpose is to output a single variable.

In the Library-book get rid of explicit. It's needless and can cause complications later. By default you need to have a standard constructor. And then you can overload it. If you call the overload constructor, then it only uses the overload. You don't have to force it to use just that one... when you aren't going to write for the other. The only time you'd actually do that is when the library is expected to be used by people who don't know what they are doing.

 

Move the constructors into public. It's not a problem as far as I am aware of... but it's less annoying to be able to call it directly when you need it.

 

 

Annnd... I guess that data redundancy is not something you need to worry about? 

Edited by Tangletail

Share this post


Link to post
Share on other sites

I am very much interested in using smart pointers so I'd appreciate any advice especially if shared_ptrs should/can be cast to weak_ptrs. This is not homework, this is a practice project from here: http://www.cppforschool.com/projects.html If I wanted to copy and paste code I would just copy the code they supply there and not post here. As you'll see I am taking a far more strict approach to memory management and general design for my own benefit and knowledge.

 

What are the complications that can occur from using explicit?

 

Please elaborate on data redundancy and where this occurs in the code. I haven't come across this term before and googling didn't clear up where I am making this mistake.

Edited by HiMK

Share this post


Link to post
Share on other sites

You need to think about object lifetime and ownership, proceeding systematically from requisites to solutions; choosing appropriate techniques to manage memory among various types of smart pointers, rules for constructors and destructors, container ownership etc. is one of the important points of any C++ programming exercise.

Share this post


Link to post
Share on other sites

I am doing a beginner C++ project and its simply a Library Mangement System where a student will hire, give back a book, book and hire records are stored in a binary file etc.
...
LibraryBook features I want to implement:

- The class has the expected private properties of title, author, availability, id. I have made the LMS a friend class so that it and only it can change the books availability, maybe even author, title (for scalability/extendability). Would you use friend here or maybe another technique?

Remember, a book is not a hire record.  I see no reason to use friends at all here.
 
A Student asks the LMS for a Book.  Internally, the LMS has a list of HireRecords which join the Students with the Books (with a date attribute).  It knows a Student can not hire a Book if there is a currently valid HireRecord for that Book, and since a student can only hire 2 books at a time, will also refuse the Student's request if there is more than two currently valid HireRecords for the Student (I made that last up to illustrate the flexibility of the system, and because I remember that from when I was a child).  If you save the HireRecords you can generate reports such as who has overdue books, or which Student hired which Book when for the Authorities during their Ongoing Investigation.
 
So, no, no friend required.  Just classes based on data reduced to third normal form.

Edited by Bregma

Share this post


Link to post
Share on other sites
Just to challenge the habit nice and early, do you really need to use that Meyers Singleton approach to the LibraryManager? Why does this need to be constrained to one only? Why can't it just be a local variable in your main() that gets passed around to functions that need to access it?

void borrow(LibraryManager &m)
{
    m.doStuff();
}

void main()
{
    LibraryManager m;
    borrow(m);
}
This might seem trivial now, but as you progress, you'll hopefully come to see that global state is a horrible, horrible thing.

Good luck. Edited by Aardvajk

Share this post


Link to post
Share on other sites

I think you are massively over-complicating things with the idea that that will make them "scalable" and "easier to use." This is generally not the case; simplicity is usually preferable. You also seem to have some misconceptions about how certain C++ features work. I'll cover those first:

 

A LibraryBook must have its title, author, etc. defined always. So I have made the constructor explicit. Should I make the default constructor private?

 

 

That's not what explicit does. If a book must always have a title and author, make sure there is a constructor that takes a title and author. Don't provide any constructors without those parameters. The explicit specifier does not factor into that decision; explicit is about preventing constructors or conversion operations from being used in implicit conversions.

 

I can give the Student a weak_ptr to the LibraryBook which means they cant change anything on that object.

 

 

A weak_ptr does not prevent modification of the pointed-to object. A weak_ptr is a pointer to a object managed by a shared_ptr that does not contribute to the shared reference count (in other words, a weak_ptr doesn't keep an object alive). Modification of the pointed-to object is still possible through a weak_ptr, unless that weak_ptr is const (but that applies to all pointer types).

 

Turning to your design goals and decisions themselves:

 

The class has the expected private properties of title, author, availability, id. I have made the LMS a friend class so that it and only it can change the books availability, maybe even author, title (for scalability/extendability). Would you use friend here or maybe another technique?

 

 

No reason for using "friend" here. The title and author should be private, but have public accessors to read them. "Availability" of a book is not a property of a book itself, but a property of a library: one asks the library "do you have <this book> available?" and it tells you yes or no. There's no reason to provide for changing a book's title or author. That doesn't happen and doesn't make much sense within the context of the object model you've chosen.

 

A book object should only ever be created by the LMS, this makes sense I guess because a Student shouldn't be able to make a new book only the LMS. So I was thinking of making the LibraryBook class private but that means a Student wont even be able to hold a book (as a property in their class). I am thinking I should only ever give a handle of the book to the Student? Maybe this is the case for the application of the factory design pattern.

 

 

This is a pointless restriction. Anybody can create a book. You can then donate that book to the library. There is consequently no reason to keep the book constructors private, or to make the book class itself private. Even if you were going to do this, this is not what the factory pattern is for. The factory pattern is about creating instances of things based on data, without necessarily specifying the precise type of thing in code. In this case you are only ever dealing with books, you don't need a cumbersome factory sitting there overcomplicating things.

 

const unsigned int ID;

 

 

You don't use this (in the book class). And you probably don't need it unless you're trying to model the ISBN system.

 

 

static LibraryManagementSystem& Instance()

 

 

I've got news for you: there's more than one library in the world. Don't do this.

 

weak_ptr<LibraryBook> hireBook(string bookTitle, Student& student)

 

 

A few things here:

  • You can build this system without using pointers at all; books can be value types you simply move around (out of the library's inventory and into the students). This will simplify the system. If you really desperately want to use pointers for practice, this is not really a great scenario for weak_ptr. A book's ownership can reasonably be shared; if I check out a book from a library, and that library is bulldozed the next day, I still have that book. What you've currently got is more complex than you can necessarily cope with right now; I'd use a simpler implementation until you really understand what's going on.
  • The student should be able to have more than one book out at a time. That's a thing that can happen. Consider instead giving the student a list of books and calling an "add book" method here. See below for other notes on "curBook."
  • Generally if you are going to modify a parameter (the student), it is stylistically preferential to pass that parameter via a pointer, because it's more obvious at the call site that something might happen to it.

 

    weak_ptr<LibraryBook> curBook; // is this ok as public? or should only the LMS be able to set a students book? Maybe this property only should be friend with LMS?

 

 

 
This should not be public and it should not be singular. Make it a private vector of books and put it behind a public interface for querying or manipulating the list. 
 
You can't make "properties" friends, by the way. Friendship's granularity is the type.

// what should I return? An empty/null weak_ptr?
 

 

Null.

 

~LibraryManagementSystem() {}

 

 

As above, this shouldn't be a singleton, but even if it is the destructor shouldn't be private.
Edited by Josh Petrie

Share this post


Link to post
Share on other sites

I am very much interested in using smart pointers so I'd appreciate any advice especially if shared_ptrs should/can be cast to weak_ptrs. This is not homework, this is a practice project from here: http://www.cppforschool.com/projects.html If I wanted to copy and paste code I would just copy the code they supply there and not post here. As you'll see I am taking a far more strict approach to memory management and general design for my own benefit and knowledge.

 

What are the complications that can occur from using explicit?

 

Please elaborate on data redundancy and where this occurs in the code. I haven't come across this term before and googling didn't clear up where I am making this mistake.

 

Please note that the indirection was from a lack of communication. Also, I called this homework, because I had to do this mess in college. Though at a much more infuriating level.

If it's your gall to use smart pointers, go for it. But it's over complicating things.

Now then... to answer the questions.

Data Redundancy is a poorly chosen term by egg heads. Redundancy can mean a variety of things in programming. Basically, you are simply storing data to a disk, and loading from it at the start of the program and end, or by menu choice. After all, it's bad software design if you loose data because the computer crashed, and you couldn't save. The use of pointers makes an easy task of linearly writing everything to a file more annoying.

Back to the pointer situation. Now, I don't mean to point fingers here, but the problem is the weak_ptr in the map. It's designed to be a temporary method of storing data. And it can possibly expire at a moments notice. How it expires, is dependent on the code design.

Instead, you will want to use std::shared_ptr in the map. The shared pointer can delete it's self, but only by reference count, and it's reference never changes. This means that as long as the map exists, and the map is referring to it, then it stays.

 

To help explain this. Imagine that a dumb pointer is a person who follows orders directly. And that a smart pointer is Monty Python's Suicide Squad. And a weak smart pointer is a oneshot rifle. The people f follows orders. But the dumb one is not sucidal, while the smart one... when he has nothing to loose will kill himself in front of the enemy. The oneshot rifle remains mounted on a stick of butter to never be directly used... because using it will be messy. This is how pointers do. And this is the actual explanation given to me by my professor.

 

The weak pointer is there to prevent circular referencing, which causes the problems of the pointers not deleting themselves.

That being said, you cast a shared_ptr to a weak pointer like normal. But you still need to cast to a weak pointer to a shared pointer to access it. It's complicated, I know. Which is why I prefer to use dumb pointers. Because while I have a loaded gun with the worlds most sensitive trigger pointed at my foot, it's less things to remember and check.

http://www.cplusplus.com/reference/memory/shared_ptr/

 

And finally, the complications that could come from explicit. The first part is... it's not doing what you think it's doing. See this link
https://isocpp.org/wiki/faq/ctors#explicit-ctors
Explicit will only work on single argument constructors. Also, the way it works is a lot like creating and casting a int. That's really the main purpose of it.

Actually... I am not even sure if it compiles with multi-argument constructors.

Edited by Tangletail

Share this post


Link to post
Share on other sites

const string firstName;
const string lastName;
const unsigned int ID;

const string title;
const string author;
const unsigned int ID;

Why are almost all your class members const?
 
 
L. Spiro Edited by L. Spiro

Share this post


Link to post
Share on other sites

 

const string firstName;
const string lastName;
const unsigned int ID;
const string title;
const string author;
const unsigned int ID;
Why are almost all your class member const?

 

Why not? When is OP ever going to change these fields? If you know that a field should never change over the lifetime of the instance, make it const to cut down on mutable state.

Edited by Oberon_Command

Share this post


Link to post
Share on other sites

 

 

const string firstName;
const string lastName;
const unsigned int ID;
const string title;
const string author;
const unsigned int ID;
Why are almost all your class member const?

 

Why not? When is OP ever going to change these fields? If you know that a field should never change over the lifetime of the instance, make it const to cut down on mutable state.

 

 

 

When data needs to be edited. Usually Name and Address. Well... just about everything just in case of errors.

Edited by Tangletail

Share this post


Link to post
Share on other sites

Why not?

CMyObj & CMyObj::operator == ( const CMyObj &_moOther ) {
    title = _moOther.title; // Error.
    const_cast<string &>(title) = _moOther.title; // Too much code, hacky.
    return (*this);
};

It may make sense for some objects to be create-once-and-never-mutate, but typically in those circumstances we just make those objects themselves const.

It doesn’t make sense to force every single instance of a class to be fully immutable after construction.

 

 

L. Spiro

Share this post


Link to post
Share on other sites

 

 

 

const string firstName;
const string lastName;
const unsigned int ID;
const string title;
const string author;
const unsigned int ID;
Why are almost all your class member const?

 

Why not? When is OP ever going to change these fields? If you know that a field should never change over the lifetime of the instance, make it const to cut down on mutable state.

 

 

 

When data needs to be edited. Usually Name and Address. 

 

I don't see an address field here. I suppose a library patron might want to change their name, but what data are you going to edit besides that? Books rarely change their name, author, or ISBN, and the patron's ID will never change in the lifetime of the record, much less the program's runtime. Even with "errors," by which I assume you mean user mistakes.

 

It may make sense for some objects to be create-once-and-never-mutate, but typically in those circumstances we just make those objects themselves const.

It doesn’t make sense to force every single instance of a class to be fully immutable after construction.

 

Fair point.

 

The student should be able to have more than one book out at a time. That's a thing that can happen.

 

Because I didn't see this mentioned earlier: the project description OP is working from specifies that students can only ever have one book out at a time. I agree that it's a pretty stupid requirement, but then again, this is the same project that uses example C++ that must have been written in the early '90s. Good for you, OP, in not bothering with the example code and trying to do better.

Edited by Oberon_Command

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this