Parsing store sales content from a text file

Question

I asked this question yesterday, and now I have come up with this code:

stock.h

#ifndef stock_stock_h
#define stock_stock_h
#include <iostream>

class  stock  {

public:
    stock() {
        itemName = " ";
        unitPrice = " ";
        quantityPurchased = " ";
        day = " ";
        month = " ";
        year = " ";
    }

    stock(std::string itemName,std::string unitPrice,std::string quantityPurchased,
          std::string day,std::string month,std::string year);

    std::string getItemName();
    std::string getUnitPrice();
    std::string getQuantityPurchased();
    std::string getDateDay();
    std::string getDateMonth();
    std::string getDateYear();


    void setItemDescription(std::string itemName);
    void setUnitPrice(std::string unitPrice);
    void setQuantityPurchased(std::string quantityPurchased);
    void setDateDay(std::string day);
    void setDateMonth(std::string month);
    void setDateYear(std::string year);


private:
    std::string itemName,unitPrice,quantityPurchased,day,month,year;
};
#endif

main.cpp

#include "stock.h"
#include <iostream>
#include <string>
#include <fstream>
#include <sstream>
#include <iterator>
#include <cassert>
#include <vector>
#include <iomanip>

using namespace std;

stock::stock(string itemName,string unitPrice,string quantityPurchased,string day,string    month,string year)  {

    setItemDescription(itemName);
    setUnitPrice(unitPrice);
    setQuantityPurchased(quantityPurchased);
    setDateDay(day);
    setDateMonth(month);
    setDateYear(year);

};

string stock::getItemName()    {
    return itemName;
}
string stock::getUnitPrice()  {
    return unitPrice;
}

string stock::getQuantityPurchased()  {
    return quantityPurchased;
}

string stock::getDateDay()  {
    return day;
}

string stock::getDateMonth()  {
   return month;
}

string stock::getDateYear()  {
   return year;
}

void stock::setItemDescription(std::string itemName) {
    this->itemName = itemName;
}

void stock::setUnitPrice(std::string unitPrice) {
    this->unitPrice = unitPrice;
}

void stock::setQuantityPurchased(std::string quantityPurchased) {
    this->quantityPurchased = quantityPurchased;
}

void stock::setDateDay(std::string day) {
    this->day = day;
}

void stock::setDateMonth(std::string month) {
    this->month = month;
}

void stock::setDateYear(std::string year) {
    this->year = year;
}

int main(){
    vector<stock> itemDetails;
    string line;
    string itemName;
    string unitPrice;
    string quantityPurchased;
    string inputMonth;
    string day;
    string month;
    string year;
    double uPrice;
    int qPurchased;
    double totalIndividualItemSales;
    double totalSalesOfMonthDec = 0.0;
    double totalSalesOfMonthJan = 0.0;
    double totalSalesOfMonthFeb = 0.0;

    //put your text file name inside the ""
    ifstream readFile("/Users/jeremykeh/Desktop/stock.txt");

    while(getline(readFile,line))   {
        stringstream iss(line);
        getline(iss, itemName,':');
        getline(iss, unitPrice, ':');
        getline(iss, quantityPurchased, ':');
        getline(iss, day, '-');
        getline(iss, month, '-');
        getline(iss, year, '-');

        //consturctor

        stock splitedColumns(itemName,
                             unitPrice,
                             quantityPurchased,
                             day,
                             month,
                             year
                             );
        itemDetails.push_back(splitedColumns);

    }
    readFile.close();

    cout << "Info reterived from file" << endl;

    for (int i =0; i<itemDetails.size(); i++) {

        if(itemDetails[i].getDateMonth() == "Dec" && itemDetails[i].getDateYear() == "2013")  {
            istringstream uPbuffer(itemDetails[i].getUnitPrice());
            istringstream qPbuffer(itemDetails[i].getQuantityPurchased());

            uPbuffer >> uPrice;
            qPbuffer >> qPurchased;
            totalIndividualItemSales = uPrice * qPurchased;
            totalSalesOfMonthDec += totalIndividualItemSales;

        }

        if(itemDetails[i].getDateMonth() == "Jan" && itemDetails[i].getDateYear() == "2014")  {
            istringstream uPbuffer(itemDetails[i].getUnitPrice());
            istringstream qPbuffer(itemDetails[i].getQuantityPurchased());

            uPbuffer >> uPrice;
            qPbuffer >> qPurchased;
            totalIndividualItemSales = uPrice * qPurchased;
            totalSalesOfMonthJan += totalIndividualItemSales;

        }

        if(itemDetails[i].getDateMonth() == "Feb" && itemDetails[i].getDateYear() == "2014")  {
            istringstream uPbuffer(itemDetails[i].getUnitPrice());
            istringstream qPbuffer(itemDetails[i].getQuantityPurchased());

            uPbuffer >> unitPrice;
            qPbuffer >> quantityPurchased;
            totalIndividualItemSales = uPrice * qPurchased;
            totalSalesOfMonthFeb += totalIndividualItemSales;

        }

    }
    cout << "Report Summary of sales(Monthly)" << endl;
    cout << setw(10) << left << "Month" <<setw(5) << "Sales" <<endl;
    cout <<setw(10) << left << "Dec 13" <<setw(5) << totalSalesOfMonthDec <<endl;
    cout <<setw(10) << left << "Jan 14" <<setw(5) << totalSalesOfMonthJan <<endl;
    cout <<setw(10) << left << "Feb 14" <<setw(5) << totalSalesOfMonthFeb <<endl; 
}

Although I've achieved the results I wanted, I feel that I am hard-coding it badly to achieve my results. I wish to know/learn how I can further improve my code from you all.

My output

Info reterived from file
Report Summary of sales(Monthly)
Month     Sales
Dec 13    16   
Jan 14    46   
Feb 14    25

Answer 1

• stock doesn't need a default constructor if the starting values won't be useful. If you'll never need to create a stock object with default values, then you can just leave this out.

• In private, declare each member once per line:

  std::string itemName;
  std::string unitPrice;
  std::string quantityPurchased;
  std::string day;
  std::string month;
  std::string year;
  

  This should also be done in general as it helps maintain readability and maintainability.

• Consider finding an alternative to all those accessors and mutators. They're not good for encapsulation as they can expose implementation details. It's best to have member functions that modify these data members within the class.

• It seems that qualtityPurchased, day, month, and year should be of integer types, unless they specifically need to be strings.

• It looks like you're including both stock's implementation and main() in main.cpp. As the file name suggests, only main() should be in this file. There should then be an additional .cpp file just for stock's implementation.

• After you construct a stock object in main(), it's not quite clear what you're doing. Consider putting the contents of that for loop in a function with a descriptive name. The output after that could also be put into a separate display function or just stay in main().

Answer 2

Agree with everything Jamal said.

In addition.

• Consider finding an alternative to all those accessors and mutators.

  ---

  Can't emphasis enough how bad this makes your design. get/set pattern is terrible for breaking encapsulation. You are exposing the internal types and thus tightly binding your implementation to providing those types.

  ---

  Your object should be self contained with actions that can be performed on the type represented as methods (verb like) that interact with the object.

• Write a stream operator for your class

  ---

  This will make your reading of the object extremely compact.

  ie:

  std::istream& operator>>(std::istream& stream, stock& item) {
      // Your stuff for reading in here.
  }
  

  This makes the code much simpler to write.

  vector<stock> itemDetails;
  stock   item;
  while(readFile >> item))   {
      itemDetails.push_back(splitedColumns);
  }
  

  Or if you want to look bad ass you can then use tricks to do it in one line.

  // Or even this single line.
  vector<stock> itemDetails(std::istream_iterator<stock>(readFile), std::istream_iterator<stock>());
  

• Distinguish type names from variable/member names.

  ---

  Personally I use an initial capitol letter for all my types (everything else starts with a lower case letter). It makes it easier to spot a type. But that is just a personal preference.

• Declare variables as close to the point of usage as possable. This is not C you don't need to put everything at the top of a function. Declare them just before you use them. This locality of use helps you spot the type of a variable without having to scroll around looking for it.

• If you must use getters. declare them const. And return a const ref to the internal member. That way you prevent copying.

  string const& stock::getDateYear() const
  {   // ^^^^^^  const reference.    ^^^^^   the function is const.
      return year;                           i.e. does not mutate the class.
  }
  

• Why are you storing the year as a string?

  ---

  Store time as a unix timestamp (in UTC time. Convert it to the appropriate time zone for display but store all times as UTC)