patterncppMinor
Calculating highs, lows, and volumes of stocks
Viewed 0 times
highslowsvolumescalculatingstocksand
Problem
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 string;
class CHighLow
{
public:
CHighLow() : nCurLow(0), nCurHigh(0) {};
void add(int nHigh, int nLow)
{
if (nHigh > nCurHigh)
nCurHigh = nHigh;
if (nLow TotalVolumes;
for (int s = 0; s HighLows;
for (int s = 0; s ::iterator itr = HighLows.begin();
while (itr != HighLows.end())
{
cout << (*itr).first << "," << (*itr).second.nCurLow << "," <<
(*itr).second.nCurHigh << endl;
++itr;
}
return 1;
}Solution
First, the obvious ones:
-
Change
-
Change
to
and
to
because initializing min/max with 0 can lead to bugs if values are all >0 or
-
Drop
-
Check
-
Don't use
-
Don't increment
-
The iteration looks bogus,
-
More error checking.
There could be other things, but having some test data would be nice. There could be soft bugs in the code.
-
Change
nCurLow and nCurHigh to floats since you are reading float from file.-
Change
nCurLow(0)to
nCurLow(std::numeric_limits::infinity())and
nCurHigh(0)to
nCurHigh(-std::numeric_limits::infinity())because initializing min/max with 0 can lead to bugs if values are all >0 or
-
Drop
typedef basic_string string; and use std::string instead.-
Check
argc if you have arguments before you use argv (prevents crash when invalid arguments are passed).-
Don't use
cerr only when you want to report error, use std::cout instead.-
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 ...).-
The iteration looks bogus,
for (int s = 0; s
-
Avoid stdcmp; use operator== from std::string.
These points only refer to potential errors and bugs.
Now for the less obvious ones:
-
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
-
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.
-
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.
-
It would be nice if there would be some comments to describe what the code actually does.
-
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).-
More error checking.
There could be other things, but having some test data would be nice. There could be soft bugs in the code.
Code Snippets
nCurLow(std::numeric_limits<float>::infinity())nCurHigh(0)nCurHigh(-std::numeric_limits<float>::infinity())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];
}
}
}Context
StackExchange Code Review Q#86145, answer score: 3
Revisions (0)
No revisions yet.