Calculating highs, lows, and volumes of stocks

I'm trying to review and figure the issues in the above code. Any inputs to point out problems are welcome.

using namespace std;
typedef basic_string<char> string;

class   CHighLow
{
public:
CHighLow() : nCurLow(0), nCurHigh(0) {};

{
if (nHigh > nCurHigh)
nCurHigh = nHigh;

if (nLow < nCurLow)
nCurLow = nLow;
}

int     nCurLow;
int     nCurHigh;
};

int main(int argc, char* argv[])
{
if (!strcmp("version", argv[1]))
{
cerr << "Using version 1.0 VWAPer" << endl;
return 0;
}

FILE*   file = fopen(argv[2], "r");

cerr << "Reading file" << argv[2] << endl;

char    line[256];
char    Stocks[1000][10];
int     Intervals[1000];
int     Volumes[1000];
float   Highs[1000];
float   Lows[1000];

int     i = 0;
int     sum = 0;

while (fgets(line, 256, file))
{
sscanf(line, "%s %d %d %f %f",
Stocks[i], &Intervals[i],
&Volumes[i], &Highs[i], &Lows[i++]);

cerr << Stocks[i] << endl;
cerr << Intervals[i] << endl;
cerr << Volumes[i] << endl;
cerr << Highs[i] << endl;
cerr << Lows[i] << endl;
}

cerr << "Calculating total volumes" << endl;

map<std::string, int>       TotalVolumes;

for (int s = 0; s <= i; ++s)
{
std::string stockname = Stocks[s];

for (int j =0; j <= i; ++j)
{
if (!strcmp(Stocks[j], stockname.c_str()))
{
TotalVolumes[stockname] += Volumes[j];
}
}
}

cerr << "Calculating high lows" << endl;

map<std::string, CHighLow>  HighLows;

for (int s = 0; s <= i; ++s)
{
}

cerr << "Writing files" << endl;

// write file a
for (int s = 0; s <= i; ++s)
{
cout << Stocks[s] << "," << Intervals[s] << "," <<
TotalVolumes[Stocks[s]] / Volumes[s] * 100 << endl;
}

// write file b
map<std::string, CHighLow>::iterator itr = HighLows.begin();
while (itr != HighLows.end())
{
cout << (*itr).first << "," << (*itr).second.nCurLow << "," <<
(*itr).second.nCurHigh << endl;

++itr;
}

return 1;
}

• Commented Apr 7, 2015 at 14:21
• Ok, So how does that affect the code? Commented Apr 7, 2015 at 14:31
• Hi! Welcome to Code Review. Please add more context to the question, such as explain what your code does and your major concerns. Commented Apr 7, 2015 at 15:12
• @Sammy click the link and find out. Commented Apr 7, 2015 at 15:24
• can you use a c++11 or c++14 compiler? And what are the issues you mentioned Commented Apr 7, 2015 at 15:43

First, the obvious ones:

1. Change nCurLow and nCurHigh to floats since you are reading float from file.

2. Change

nCurLow(0)


to

nCurLow(std::numeric_limits<float>::infinity())


and

nCurHigh(0)


to

nCurHigh(-std::numeric_limits<float>::infinity())


because initializing min/max with 0 can lead to bugs if values are all >0 or <0.

3. Drop typedef basic_string<char> string; and use std::string instead.

4. Check argc if you have arguments before you use argv (prevents crash when invalid arguments are passed).

5. Don't use cerr only when you want to report error, use std::cout instead.

6. Don't increment i like this: ... &Lows[i++]);. In fact drop those arrays all together and use std::vector, and std::string instead (because that while so broken in so many ways ...).

7. The iteration looks bogus, for (int s = 0; s <= i; ++s) because of <= so change it to ranged-based fors.

8. Avoid stdcmp; use operator== from std::string.

These points only refer to potential errors and bugs.

Now for the less obvious ones:

1. I see a comment //write to a file. std::cout does not write to file but to standard output. use file streams if writing to a file is the intended behavior

2. This code:

for (int s = 0; s <= i; ++s)
{
std::string stockname = Stocks[s];

for (int j =0; j <= i; ++j)
{
if (!strcmp(Stocks[j], stockname.c_str()))
{
TotalVolumes[stockname] += Volumes[j];
}
}
}


What does it actually do? It looks totally inefficient. The obvious thing is that TotalVolumes[stockname] can be evaluated only once. The second is that j can be equal to s.

3. The rest of the code seems to make excessive use of memory. It looks like it can be simplified a lot, with fewer iterations, less memory usage, less confusion.

4. It would be nice if there would be some comments to describe what the code actually does.

5. return 1 in main. If this will be used in an automated tool return 1 might mean that an error has occurred. Change it to return 0 and return 1 only when an actual error has happened (invalid argument, file not found, corrupted file data, etc).

6. More error checking.

There could be other things, but having some test data would be nice. There could be soft bugs in the code.

This code doesn't catch any buffer overflows.
What happens e.g. if

• the file contains more than 1000 lines
• a line is more than 256 characters long
• the name of a stock more than 10
• the number of command line parameters is less than 2

And if I'm not completely mistaken, then

for (int s = 0; s <= i; ++s)
{
std::string stockname = Stocks[s];

for (int j = 0; j <= i; ++j)
{
if (!strcmp(Stocks[j], stockname.c_str()))
{
TotalVolumes[stockname] += Volumes[j];
}
}
}


can be replaced by

for (int s = 0; s <= i; ++s)
{
TotalVolumes[Stocks[s]] += Volumes[s];
}


Suggestion 1

The line

    sscanf(line, "%s %d %d %f %f",
Stocks[i], &Intervals[i],
&Volumes[i], &Highs[i], &Lows[i++]);


is not right. i++ can be executed first, last, or anywhere in between. The standard does not specify the order of evaluation of arguments.

Also, you are incrementing i in that statement followed by:

    cerr << Stocks[i] << endl;
cerr << Intervals[i] << endl;
cerr << Volumes[i] << endl;
cerr << Highs[i] << endl;
cerr << Lows[i] << endl;


which will print the values using an index that is off by one.

I would recommend converting the while loop to a for loop:

for ( i = 0; fgets(line, 256, file); ++i)
{
sscanf(line, "%s %d %d %f %f",
Stocks[i], &Intervals[i],
&Volumes[i], &Highs[i], &Lows[i]);

cerr << Stocks[i] << endl;
cerr << Intervals[i] << endl;
cerr << Volumes[i] << endl;
cerr << Highs[i] << endl;
cerr << Lows[i] << endl;
}


This resolves both issues.

Suggestion 2

The member function CHighLow::add seems a misnomer to me. What's being "added" in the function? A better name would be CHighLow::update. I would go further and create two functions updateHigh and updateLow.

  void updateHigh(int nHigh)
{
if (nHigh > nCurHigh)
nCurHigh = nHigh;
}

void updateLow(int nLow)
{
if (nLow < nCurLow)
nCurLow = nLow;
}


That is probably my personal preference to keep functions as simple as possible and build from there.

Suggestion 3

Use of i to store the number of entries in the file seems a strange choice. Most of us are used to using i as a looping variable. I would recommend using numEntries or numRecords instead of i for keeping track of the number of entries in the file.