trjhtr

I would like to improve my skill on C++ concepts and I came across the following question in StackOverflow, which has been directly asked by a StackOverflow-user without any preliminary attempts.(unfortunately, I have only the image of the question, since the actual post was an unsolved question, it has been removed from the site by the user itself)

Any tips or comments to optimize or improve the code and the correctness issues of the solution would be most welcome.
.

#include <iostream>
#include <string>
#include <unordered_map>

/** Class which has been asked in the question. In short Iteam class will act like
 a struct for iteamList class, since I have declared iteamList class as a friend
 of Iteam class **/
class iteamList; // forward delaration of iteamList class
class Iteam

private:
 int m_code;
 int m_price;
 int m_quantity;
public:
 Iteam() // Defualt
 :m_code(0), m_price(0), m_quantity(0)
 
 Iteam(const int& code, const int& price, const int& quntity)// Parameterized
 :m_code(code), m_price(price), m_quantity(quntity)
 
 Iteam(const Iteam& rhs) // Copy
 :m_code(rhs.m_code), m_price(rhs.m_price), m_quantity(rhs.m_quantity)
 
 Iteam(Iteam&& rhs) // Move
 :m_code(rhs.m_code), m_price(rhs.m_price), m_quantity(rhs.m_quantity)
 
 ~Iteam() 

 //(i) input the data members
 inline void inputData()
 
 std::cout<<"Iteam code :"; std::cin>>m_code;
 std::cout<<"Iteam price :"; std::cin>>m_price;
 std::cout<<"Iteam quantity :"; std::cin>>m_quantity;
 
 // life would be easy after making this statement :) !
 friend iteamList;
 //(ii) to display the data members
 friend std::ostream& operator<<(std::ostream& output, const Iteam& obj);
;
std::ostream& operator<<(std::ostream& output, const Iteam& obj)

 return output<<"Iteam code = "<<obj.m_code
 <<"t price = "<<obj.m_price
 <<"t quantity = "<<obj.m_quantity<<"n";

/******************************************************************************/
/** Implementation of the main class which is independent of the above one.
 It can access all members of above class if it has instantiated an object of it.
 I have used STL std::unordered_map to store the Items which will be easier to
 look up(with an O(1)) at the time of queries of deletion.**/

class iteamList

private:
 int m_totalStockValue = 0;
 std::unordered_map<int, Iteam> m_iteamMap;
private:
 // (iii) appending iteam to the list
 inline void AppendIteams(const int& userInput_n)
 
 for(auto i=0; i<userInput_n; ++i)
 
 Iteam obj;
 obj.inputData();
 std::cout << obj << std::endl;
 m_iteamMap.emplace(obj.m_code, obj);
 
 
public:
 iteamList() = default;

 iteamList(const int& userInput_n)
 
 m_iteamMap.reserve(userInput_n);
 AppendIteams(userInput_n);
 
 ~iteamList()

 // (iv) delete the iteam from the list
 void deleteIteam(const int& code)
 
 if(m_iteamMap.find(code) != m_iteamMap.cend())
 
 std::cout << m_iteamMap[code] << " has been deleted!n";
 m_iteamMap.erase(m_iteamMap.find(code));
 
 else
 std::cout <<"Invalid Iteam code!" << std::endl;
 
 // (v) total value of the stock
 void claculateStockValue()
 
 m_totalStockValue = 0;
 for(const auto &it:m_iteamMap)
 m_totalStockValue += (it.second.m_price * it.second.m_quantity);

 std::cout <<m_totalStockValue<< std::endl;
 
;
/**********************************************************************************/
int main()

 int number;
 std::cout <<"Enter total number of iteams : ";
 std::cin>>number;

 iteamList obj(number);

 std::cout <<"Total Value of stock = ";
 obj.claculateStockValue();

 int code;
 std::cout <<"Enter code to delete the iteam : ";
 std::cin>>code;
 obj.deleteIteam(code);

 std::cout <<"Total Value of stock = ";
 obj.claculateStockValue();

 return 0;

1. First off all: Please fix your spelling. The functionality you are trying to implement seems to be about items, so your classes should be called Item and ItemList, respectively. Also, there is at least one place in your code where you misspelled "quantity" as "quntity". claculateStockValue should be calculateStockValue. I know that it's hard for a non-native speaker to get such things right (I'm not a native speaker myself, so I understand your struggle). However, to make your code clean and easy to read, it is important to keep a certain level of cleanliness throughout naming and comment writing.


2. Speaking about name cleanliness, keep your capitalization consistent. Why does iteamList start with a lowercase i while Iteam starts with an uppercase? Decide on one naming scheme and stick with it. The same applies to your method names.


3. Instead of defining a default constructor for Iteam that does nothing special, assign default member initializers and = default that constructor (just as you did in your other class).


4. You do not need to define a copy and move constructor in Iteam. The compiler is nice enough to generate them for you automatically in most cases. You only need to define them yourself if your class does some fancy resource managing, or something alike. The same is true for the destructor.


5. Don't take int as a const reference. Take it by value instead. I don't know of a single architecture out there where and int could not be passed efficiently in a register (or on the stack).


6. You do not need to forward declare iteamList. Instead, you can just turn friend iteamList; into friend class iteamList;.


7. Actually, this is a case of friend-declaration abuse. Instead of making iteamList a friend of Iteam, Iteam should define an actual public interface (i.e. getters and setters). As of now, the whole Iteam class is somewhat useless.


8. Don't use inline unless you know exactly what you are doing. In most cases, the compiler is going to ignore it anyway.


9. Don't define inputData as a member of Iteam; it violates the single responsibility principle. Reading and parsing data is not part of the responsibility of Iteam.


10. 
    iteamList manages it's content using a... std::unordered_map?! Hopefully, you realize that this is a blatant mismatch between name and functionality: When I see a class that has "list" as part of its name, I expect it to use some kind of list to store its content.


11. That being said, std::unordered_map does not seem like a very good fit here. Indeed, a list would suggest itself very nicely if you want to keep the O(1) deletion and insertion property. Also, the identifier is already part of the Iteams; it is redundant to add it as a key to a map. If you want to keep the map, you should extract the key from the Iteam class.


12. Again, methods such as AppendIteams violate the single responsibility principle and must be extracted from your class. The responsibility of iteamList is managing a list of items; reading in and creating items is the responsibility of a different piece of code.


13. Furthermore, don't output debug messages from your class directly. This, again, violates the SRP.


14. Keep spacing around operators consistent, especially around << and >>. In general, it is considered good practice to leave a space before and after all operators.


15. As with Iteam, iteamList severely lacks a public interface. Please add getter and setter methods in exchange for all those IO-methods.


16. Don't keep m_totalStockValue around as a member. Make it local to claculateStockValue instead and return it.