Find the total volume traded for each stock and also find the high and low for each of those stocks

Question

I would like to know whether the following code can be changed in any way as to improve its functionality. Could there be any potential bugs that may arise as a result of how the code is written?

The program is designed to find the total volume traded for each stock and to also find the high and low for each of those stocks.

Are there any lines of code that are poorly written? Either syntactical or just a better way to express the logic.

#include <cstdio>
#include <cstring>
#include <map>
#include <iostream>
#include <string>


using namespace std;

typedef basic_string<char> string;

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

    void    add(int nHigh, int nLow)
    {
        if (nHigh > nCurHigh)
            nCurHigh = nHigh;

        if (nLow < nCurLow)
            nCurLow = nLow;
    }

    int& getSum()
    {
        int sum = nCurLow + nCurHigh;
        return sum;
    }

    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 << "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)
    {
        HighLows[Stocks[s]].add(Highs[s], Lows[s]);
        cout << HighLows[Stocks[s]].getSum();
    }

    cerr << "Writing files" << endl;

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

    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;
}

Answer 1

I see a number of things that could help you improve your code.

Don't abuse using namespace std

Putting using namespace std within your program is generally a bad habit that you'd do well to avoid.

Don't redefine standard names

This definition is a problem:

typedef basic_string<char> string;

It redefines std::string (because you've used the global namespace). Better would be to simply use std::string without redefining it.

C++ is case sensitive

Your code includes a line:

Map<std::string, int> TotalVolumes;

There is no Map, but there is std::map. Write the line like this instead:

std::map<std::string, int> TotalVolumes;

Don't rely on unspecified execution order

The code currently contains this:

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

The problem is that it appears you're assuming that i is incremented after the sscanf completes, but that's not guaranteed by the standard and the code is ambiguous. Instead, write it like this:

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

Use const where possible

The CHighLow::getSum function does not (and should not) alter the underlying data structure and should therefore be declared const.

Don't return a reference to a local variable

The CHighLow::getSum() function is written as returning a reference to an int. In addition to this being somewhat unusual, in this case, it's also incorrect. The reason is that it's returning a reference to sum which is a local variable within the function. When the function returns the variable goes out of scope and is destroyed, so the reference is now pointing to unallocated memory. That's a serious bug! Instead, just return an int as in:

int getSum() const
{
        return nCurLow + nCurHigh;
}

Eliminate unused variables

Unused variables are a sign of poor code quality, so eliminating them should be a priority. In this code, sum is set to 0 within main() but then never actually used. My compiler also tells me that. Your compiler is probably also smart enough to tell you that, if you ask it to do so.

Check array length before dereferencing

If the user fails to give the program command line arguments, this line will fail:

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

It would fail because there might might not be any argv[2] so you should check the argc value first and exit with an error, perhaps telling the user what the program requires, if there aren't enough arguments.

Make member data private

The the two data members of the CHighLow class are public so any function or code can alter their contents. This is not generally good design. Instead, make them private. Because the only instance their needed is for output, that can be neatly solved with the next suggestion.

Write a custom stream inserter for a class

The code currently contains this:

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

        ++itr;
}

There are a number of things that could be improved here. First, use a "range-for" and second, use a custom stream inserter. That results in this code:

for (const auto &stock: HighLows) {
    std::cout << stock.first << "," << stock.second << '\n';
}

Then in the class, create a custom inserter:

friend std::ostream& operator<<(std::ostream &out, const CHighLow &hilo) {
    return out << hilo.nCurLow << "," << hilo.nCurHigh;
}

Use C++ file I/O

Don't use the ancient FILE* I/O but instead use modern C++ std::fstream. The interface is neater and you can write a nice class extractor instead of having all of the details for extracting data placed within main.

Don't use std::endl if '\n' will do

Using std::endl emits a \n and flushes the stream. Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl.

Don't return nonzero from main unless there's an error

The convention in programming in C and C++ is that the returned integer from main() is considered by the operating system to be construed as an error code. For that reason, the non-error condition is signalled by returning a 0. Your code ends with return 1 which will be interpreted by most operating systems as an indication that an error was encountered. Because the compiler will automatically generate the equivalent of a return 0; at the end of main(), I'd advocate simply omitting it.