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

Started by
14 comments, last by Trienco 8 years, 2 months ago

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

Advertisement

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.


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


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


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.

'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.
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.

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?

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?

'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.

This topic is closed to new replies.

Advertisement