Sign in to follow this  

[C++]Need help with a customer & salesman question.

This topic is 672 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I recently finished some C++ practices from my teacher and he told me my answers got a classic issues.

I'm pretty new and I really can't figure it out, so, give me a hand please. Thanks.

 

Here is the question:

Describe the bad design of the following codes and redesign the classes.

class Customer
{
public:
	int	birthYear;
	bool	isCurrentMember;
	int	numberOfPurchases;

	Customer()
	{
		// Test data.
		birthYear = 1963;
		isCurrentMember = false;
		numberOfPurchases = 33;
	}

	void DisplayDetails()
	{
		printf( "Birth Year: %ld\r\n", birthYear );
		printf( "Current Member: %s\r\n", isCurrentMember ? "Yes" : "No" );
		printf( "Number Of Purchases: %ld\r\n\r\n", numberOfPurchases );
	}
};


class Salesman
{
private:
	Customer	bestCustomer;
	Customer	worstCustomer;
public:
	int id;

	Salesman( int initialID )
	{
		id = initialID;
	}
	Customer BestCustomer()
	{
		return bestCustomer;
	}
	Customer* WorstCustomer()
	{
		return &worstCustomer;
	}
};

class SalesTeam
{
public:
	Salesman salesman1;
	Salesman salesman2;
	
	SalesTeam()
		: salesman1(1000), salesman2(1001)
	{}

	void DisplayTeam()
	{
		printf( "Salesman 1: %ld\r\n", salesman1.id );
		printf( "Salesman 2: %ld\r\n", salesman2.id );
	}
};


void TestSalesmanCustomers()
{
	SalesTeam ourTeam;

	int medianSales_1 =
		(ourTeam.salesman1.BestCustomer().numberOfPurchases
		+ ourTeam.salesman1.WorstCustomer()->numberOfPurchases) / 2;

	int medianSales_2 =
		(ourTeam.salesman2.BestCustomer().numberOfPurchases
			+ ourTeam.salesman2.WorstCustomer()->numberOfPurchases) / 2;

	printf( "Salesman median sales = %ld\r\n", medianSales_1);
	printf( "Salesman median sales = %ld\r\n", medianSales_2);
}
 

And here is my answer

Header:

#ifndef _REDESIGN_H
#define _REDESIGN_H

#include <stdio.h>
#include <list> 

// Customer class
// *In the future this class should provide functions to set
// m_birthYear, m_isCurrentMember and m_numberOfPurchases values
// in case of changes.
class Customer
{
public:
	Customer( int birthYear, bool isMember, int numOfPurchases ):
		m_birthYear(birthYear),
		m_isCurrentMember(isMember),
		m_numberOfPurhchases(numOfPurchases) {}
	~Customer() {}

	// functions to get private member variables
	int		getBirthYear()			{ return m_birthYear; }
	int		getNumOfPurchases()		{ return m_numberOfPurhchases; }
	bool	checkCurrentMember()	{ return m_isCurrentMember; }
	void	displayDetails();

private:
	int		m_birthYear;
	bool	m_isCurrentMember;
	int		m_numberOfPurhchases;
};



// Salesman class
// *In the future, function to search customers by their birthYear, NumberOfPurchases and 
// memberStats could be added. 
// *Comparison in function addCustomer() could be extended to consider more
// situations include membership status.
class Salesman
{
public:
	Salesman( int initialID );
	~Salesman()		{ m_customerList.clear(); }

	Customer* getBestCustomer()		{ return m_bestCustomer; }
	Customer* getWorstCustomer()	{ return m_worstCustomer; }
	bool      addCustomer( Customer* cust )
	int		  getID();

private:
	Customer*	m_bestCustomer;
	Customer*	m_worstCustomer;
	int m_id;

	// Container to store list of customer pointers
	std::list<Customer*>	m_customerList;
	bool		m_firstTime;
};



// SalesTeam class
// *Function to remove a salesman from saleteam by id could be implemented
// in the future.
// *Function to calculte particular salesman's median sales nunmber can be implemented
class SalesTeam
{
public:
	SalesTeam()		{}
	~SalesTeam()	{ m_salesmanList.clear(); }

	bool		addSalesman( Salesman* sales );
	Salesman*	getSalesman( int id );
	void		displayTeam();

private:
	// *Container to store list of salesman pointers.
	// *Makes the class more flexible and easier to 
	// handle.
	std::list<Salesman*>	m_salesmanList;
};



#endif 

CPP:

#include "Redesign.h"

//--------------------------------------------------------------------
// Customer class implementations
void Customer::displayDetails()
{
	printf( "Birth Year: %ld\r\n", m_birthYear );
	printf( "Current Member: %s\r\n", m_isCurrentMember ? "Yes" : "No" );
	printf( "Number Of Purchases: %ld\r\n\r\n", m_numberOfPurhchases );
}



//-------------------------------------------------------------------
// Salesman class implementations
Salesman::Salesman( int initialID )
{
	m_id = initialID;
	m_firstTime = true;

	m_bestCustomer = 0;
	m_worstCustomer = 0;
}


int Salesman::getID()
{
	return m_id;
}


bool Salesman::addCustomer( Customer* cust )
{
	// Finds out best and worst customer by comparing their 
	// number of purchases
	if( cust )
	{
		// First time initials
		if( m_firstTime )
		{
			m_bestCustomer  = cust;
			m_worstCustomer = cust;
			m_firstTime = false;
		}

		m_customerList.push_back( cust );

		// comparison
		if( cust->getNumOfPurchases() >= m_bestCustomer->getNumOfPurchases() )
			m_bestCustomer = cust;
		else if( cust->getNumOfPurchases() <= m_worstCustomer->getNumOfPurchases() )
			m_worstCustomer = cust;

		return true;
	}
	else return false;
}



//------------------------------------------------------------------
// SalesTeam class implementation
bool SalesTeam::addSalesman( Salesman* sales )
{
	// Check if pointer valid
	if( sales )
	{
		m_salesmanList.push_back( sales );
		return true;
	}
	else return false;
}



Salesman* SalesTeam::getSalesman( int id )
{
	auto it = m_salesmanList.begin();
	while( it != m_salesmanList.end() )
	{
		if( (*it)->getID() == id )
			return (*it);
		++it;
	}
	return 0;
}



void SalesTeam::displayTeam()
{
	auto it = m_salesmanList.begin();
	while( it != m_salesmanList.end() )
	{
		printf( "Salesman: %ld\r\n", (*it)->getID() );
		++it;
	}
}
 

Thank you guys

Edited by 50_Cal

Share this post


Link to post
Share on other sites

Major error in lines 14-17. The edited class for customer implies that it receives data from an inherited class. However the class does not have a parent. Typically that'd compiler error... or at least that I would think so.

 

It should just be.

Customer( int birthYear, bool isMember, int numOfPurchases )

I hope the formatting of the entire thing is just an accident. Because it's just awful to look at.

 

For salesman, the key design flaw is that the salesman stores a pointer of the customer. This becomes an issue when the customer is suddenly deleted, that salesman will then continue to have a pointer that goes no where. The other problem is that a class has a dependency on the existence of a completely different class. If you wish to reuse the salesman in another application, you can't without changing the required data types seen there.

This is a non-issue should Salesman be pointing to it's self or a very similar interface via inheritance. Example both a Customer and a Salesman will have a name and an ID that they can be refereed to. The two can have a baseclass that provides a very standard connection between the two, thus eliminating the coupling where crap breaks when one class changes.

 

Another issue is that Customer Class has no means of identification. Kinda wonky system when you need to look up someone... and the only thing you can refer to them is March 18 1950. But oh wait... infinitely many people can have that very birthdate.

 

For salesman team, when you add a salesman, you have named the variable "Sales". That is going to get incredibly confusing very fast. Try using the word "Associate".

 

Your list stores pointers. This is fine and all... but a List is a linked list. Which means that it stores on the heap, and data is linked to each other via pointer already. This is already unnecessary... but if we look back carefully we notice a small redundancy. This class has very little in the way of specific function. All it does is store, and get. This is Ok if this class was more specific to do things only Salesmans can do. But right now, it's not ok.

We need to add a Salesman to the Salesman team. We come into the problem of coupling once again. This class is not very reusable at the very start. We can reuse the same class to also display a database of customers, and store a database of customers. We can make it a template, or to limit what the class is supposed to handle, use a common interface. You can also use an Enumeration, to determine what the instantiated class is allowed to take.

Share this post


Link to post
Share on other sites


Major error in lines 14-17. The edited class for customer implies that it receives data from an inherited class. However the class does not have a parent. Typically that'd compiler error... or at least that I would think so.

Is there a reply or @ function here?

 

Thanks for the advises, they are extremely helpful to me.

One thing, imo, for the line 14-17. I thing it's fine to write a constructor in this way, it's just and other way to initial some member variables. It works on Visual Studio

Thanks again, I'm going to redesign the thing biggrin.png

Share this post


Link to post
Share on other sites

 


Major error in lines 14-17. The edited class for customer implies that it receives data from an inherited class. However the class does not have a parent. Typically that'd compiler error... or at least that I would think so.

 

This is just a member initialization list and very valid C++, see http://stackoverflow.com/questions/7665021/c-member-initialization-list

 

Huh... learn something new about the language each day. I doubt I'm going to be using that style much though.

Share this post


Link to post
Share on other sites
'That style' is highly advisable. You can easily be in a situation where default constructing a member is expensive and then assigning it a different value in the body is expensive again. Using the initializer list avoids that issue since only a single initialization and no assignment happens.

Share this post


Link to post
Share on other sites
Editing a post appears to be broken today on the browsers I tried, so and addendum to the previous post:

Never even mind situations where you absolutely have to use the initializer list, for example because a type has no default constructor or you need to initialize references.

Share this post


Link to post
Share on other sites

A few items left in your redesign version:

 

* Consider reordering your member variables by decreasing size, it makes it easier for them to be packed tightly. Currently the Customer class is likely to have a small gap in the middle with the block int, bool, int.  If you reorder them, reorder your initializer list to be in the same order.  It seems a small thing now, but when you've got enormous objects over time that can mean hundreds of bytes of wasted space per object, multiplied by thousands or many more objects. Keeping it packed tight is a good habit to be in.

 

* The Salesman parameter for initial ID and the values set in the constructor could be put into an initializer list in the header file instead. If so, they should be in the same order the variables are declared.

 

* I'd move Salesman::getId to the header. Simple accessor functions like that are best kept in the header so the optimizer can remove their overhead.

 

* You do not need to clear your list in ~Salesman() and ~SalesTeam(). Standard container classes clean themselves up automatically on destruction.
 

* You might want to mark the accessors as const since they don't modify your class contents, but that could start a religious debate on const correctness you may not be prepared to argue.

 

* I'm not sure the purpose behind the return value of addCustomer.  Basically it is returning the null/non-null status of the parameter.  Is this a requirement?

Share this post


Link to post
Share on other sites

A few items left in your redesign version:

 

* Consider reordering your member variables by decreasing size, it makes it easier for them to be packed tightly. Currently the Customer class is likely to have a small gap in the middle with the block int, bool, int.  If you reorder them, reorder your initializer list to be in the same order.  It seems a small thing now, but when you've got enormous objects over time that can mean hundreds of bytes of wasted space per object, multiplied by thousands or many more objects. Keeping it packed tight is a good habit to be in.

 

* The Salesman parameter for initial ID and the values set in the constructor could be put into an initializer list in the header file instead. If so, they should be in the same order the variables are declared.

 

* I'd move Salesman::getId to the header. Simple accessor functions like that are best kept in the header so the optimizer can remove their overhead.

 

* You do not need to clear your list in ~Salesman() and ~SalesTeam(). Standard container classes clean themselves up automatically on destruction.
 

* You might want to mark the accessors as const since they don't modify your class contents, but that could start a religious debate on const correctness you may not be prepared to argue.

 

* I'm not sure the purpose behind the return value of addCustomer.  Basically it is returning the null/non-null status of the parameter.  Is this a requirement?

Thanks man.

The usage of const is a thing that should be considered and I missed it beforetongue.png

For the last one, I just don't know why I did that yesterday. Just want to check if Customer is added?

Share this post


Link to post
Share on other sites

'That style' is highly advisable. You can easily be in a situation where default constructing a member is expensive and then assigning it a different value in the body is expensive again. Using the initializer list avoids that issue since only a single initialization and no assignment happens.

I mean the formatting in the header where there are HUUUGE unstructured gaps on the same line. But that is just my opinion.

Edited by Tangletail

Share this post


Link to post
Share on other sites


* You might want to mark the accessors as const since they don't modify your class contents, but that could start a religious debate on const correctness you may not be prepared to argue.

 

Make them const. Let's do this.

 

knights-templar.jpg

 

(Who actually argues about const correctness? That's like arguing about eating through your mouth.)

Share this post


Link to post
Share on other sites
Because const correctness quickly becomes an all-or-nothing process in games.

If you start enforcing it in a few places, suddenly you've got functions needing to ensure they are const so they call other functions and those need to be changed. Then it starts to propagate and you've got some library you don't control that operates on a read-only fashion and should be marked as const but isn't, so you need to take steps to fight const correctness because of the third party library.

It quickly becomes an all-or-nothing proposition. If you are providing a library it is less of a choice: be const correct. If you are using libraries and all of them have been careful about that, wonderful for you. But if you re using a bunch of other libraries and any of them are not using it, it quickly becomes the stuff of nightmares.

In practical terms, the main benefit of const correctness is a benefit to the programmer. It can help identify and prevent certain bugs, but assuming other good practices are in place those bugs will be prevented anyway by code tests and code review and QA testing.

Ultimately the options boil down to complete const correctness through the code base or completely avoiding const in the code. In most projects I've worked on professionally, teams have chosen the avoidance route primarily because attempts to reach const correctness are blocked by external libraries.

Share this post


Link to post
Share on other sites
printf( "Birth Year: %ld\r\n", m_birthYear );

The \r is not required, C++ itself converts platform EOL to/from \n

By leaving it out, code work at all platforms

 

One thing I seem to be missing, who deletes the customers? Clearing a list of pointers just makes the customers unreachable, they are never freed.

Share this post


Link to post
Share on other sites

I mean the formatting in the header where there are HUUUGE unstructured gaps on the same line. But that is just my opinion.


That looks reasonable to me too. One tab to indent the constructor from the class body, one tab stop to indent the initializers inside the constructor. The space per tab is a bit more than I would use myself but between the forum software being rather unreliable with formatting and the fact that it will immediately look good as soon as I have it in my own editor with my preferred tab size, I don't see the problem.

Share this post


Link to post
Share on other sites

Because const correctness quickly becomes an all-or-nothing process in games.

If you start enforcing it in a few places, suddenly you've got functions needing to ensure they are const so they call other functions and those need to be changed. Then it starts to propagate and you've got some library you don't control that operates on a read-only fashion and should be marked as const but isn't, so you need to take steps to fight const correctness because of the third party library.

It quickly becomes an all-or-nothing proposition. If you are providing a library it is less of a choice: be const correct. If you are using libraries and all of them have been careful about that, wonderful for you. But if you re using a bunch of other libraries and any of them are not using it, it quickly becomes the stuff of nightmares.

In practical terms, the main benefit of const correctness is a benefit to the programmer. It can help identify and prevent certain bugs, but assuming other good practices are in place those bugs will be prevented anyway by code tests and code review and QA testing.

Ultimately the options boil down to complete const correctness through the code base or completely avoiding const in the code. In most projects I've worked on professionally, teams have chosen the avoidance route primarily because attempts to reach const correctness are blocked by external libraries.

 

And now I'm emo. I hope you're happy.

Share this post


Link to post
Share on other sites

Nobody mentioning the bug in printf (and how it probably shouldn't be used in the first place)?

 

%ld = long, but the variable is int. This will work great, until you build that code as a 64bit build on Unix, where a long is 64bit and int is 32bit. Most compilers should even warn about this.

 

Either use %d or (safer) use cout instead of printf.

Share this post


Link to post
Share on other sites

This topic is 672 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

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