HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Calculating highs, lows, and volumes of stocks

Submitted by: @import:stackexchange-codereview··
0
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 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.