6
\$\begingroup\$

I wrote this code to analyze ~800 MB of weather data from the US. I am planning on visualizing the data with Gnuplot and Gimp. I have already made the images and a gif file. This code runs somewhat fast, although I do not suppose it is efficent. How can I improve it?

The idea behind my code is:

  • find keywords in the lines, such as state names and their weather conditions
  • save the weather condition in a map according to the state and year
  • get the yearly weather conditions to a file named after the analyzed state

main code:

int main()
{
    std::string data[18] = {"stormdata_1996.csv",
                            ... // I left out the files from her on purpose
                            "stormdata_2013.csv"};
    for(int j=0;j<18;j++){
        std::string city[36] = {"TEXAS",
                                ... // left out on purpose as well
                                "OREGON",
                                "MISSOURI"};
        for(int i=0;i<36;i++){
            std::map<std::string, int> weather;
            std::ifstream fin(data[j].c_str(),std::ios::in);
            getWeather(weather, fin, city[i]);
            fin.close();
            std::ofstream fout(city[i].c_str(),std::ios::app);
            outPut(weather, fout, city[i],j+1996);
            fout.close();
            weather.clear();
        }
    }
    return 0;
}

functions:

void getWeather(std::map<std::string, int>& weather, std::ifstream& fin, std::string& city){
    std::string line;
    while(!fin.eof()){
        getline(fin, line);
        if(line.find(city) != std::string::npos){
            if(line.find("Drought") != std::string::npos){
                weather["Drought"]++;
            }else if(line.find("Flood") != std::string::npos){
                weather["Flood"]++;
            }else if(line.find("Heavy Snow") != std::string::npos){
                weather["Heavy Snow"]++;
            ... // I left out some else if statements from here to shorten the post
            }else if(line.find("High Surf") != std::string::npos){
                weather["High Surf"]++;
            }
        }
    }
}


void outPut(std::map<std::string, int>& weather, std::ofstream& fout,std::string& city, int date){
    fout << city << "-" << date << std::endl;
    for(auto i:weather){
        fout << i.first << "\t" << i.second << std::endl;
    }
    fout << std::endl;
}

Here's a gif of ALASKA's weather from 1996 to 2013: Alaska's weather conditions yearly and number of events

\$\endgroup\$
  • \$\begingroup\$ Don't use eof as your while condition: stackoverflow.com/questions/5605125/… \$\endgroup\$ – Maikel Mar 12 '17 at 13:45
  • \$\begingroup\$ Then I can simply use while(getline(fin,line)){...} instead, can't I? That seems more logical. I suppose getline() return some kind of error if the operation was unsuccesful. Thanks! \$\endgroup\$ – Qbeer666 Mar 12 '17 at 14:12
  • \$\begingroup\$ @Qbeer666, it actually returns the std::istream& you passed into it. std::istream& has operator bool() (std::basic_ios to be exact), which returns true if the stream has no error flags set and ready for I/O. \$\endgroup\$ – Incomputable Mar 12 '17 at 17:15
4
\$\begingroup\$

I see a number of things that may help you improve your program.

Use the required #includes

The code uses std::map which means that it should #include <map>. It was not difficult to infer, but it helps reviewers if the code is complete. I believe that the full set of required includes is:

#include <string>
#include <iostream>
#include <fstream>
#include <map>

Use const where practical

The current getWeather() routine does not (and should not) modify the passed city string, and so it should be declared const:

void getWeather(std::map<std::string, int>& weather, std::ifstream& fin, 
        const std::string& city){

For the outPut routine, both city and weather should be const.

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.

Let the computer do the counting

The main() routine contains this:

std::string city[36] = {"TEXAS",
                        "OREGON",
                        // ... presumably 33 other states
                        "MISSOURI"};
for(int i=0;i<36;i++){

What would make more sense is to omit the 36 and simply iterate directly over the strings, using a "range for" since you're using C++11. It's a bit peculiar to have 36 states since the United States hasn't had 36 states since 1867, when Nebraska became the 37th state, and even more peculiar that such an array is named city! I'd write it like this:

std::string states[] = {"TEXAS",
                        "OREGON",
                        // .. maybe 47 other states?
                        "MISSOURI"};
for(const auto &state : states) {

Don't use eof for the loop condition

The code currently contains these lines:

while(!fin.eof()){
    getline(fin, line);

However, that's almost never what you really want. Instead, write this:

while (std::getline(fin, line)) {

This is not only shorter and more direct, but it actually functions in the way you'd want and expect.

Avoid repeating code

In the getWeather() routine, we have a long list of if statements that looks like this:

if(line.find("Drought") != std::string::npos){
    weather["Drought"]++;
}else if(line.find("Flood") != std::string::npos){
    weather["Flood"]++;

This approach has a few problems. First, it's needlessly repetitive since it repeats the event type twice in each pair of lines. Second, it is subject to miscounting because it assumes that, for example, the word "Flood" appearing in the line always means that the event type was "Flood". Assuming you're using this NOAA data, then it's easy to find a counterexample. For instance, if you look at the 2014 data, there was an event on 3 August 2014 for which the event code is "Thunderstorm Wind" but the description contains the word "Flood". This leads to my next suggestion.

Parse the file

An alternative to searching the entirety of each line would be to parse the lines, since it's a CSV file. There are a number of ways to do so, and a number of third-party libraries for that, but I'd be inclined to use the regex part of the standard library. In particular, this can be done with a std::regex_token_iterator.

Think carefully about data structures

Right now the code uses a std::map but since maintaining any particular ordering appears to be unimportant for this code, it would likely confer a performance benefit to use a std::unordered_map instead.

Rethink the algorithm

Right now, each input file is scanned once for every state and files are opened and closed with every iteration through the inner loop. Wouldn't it make more sense just to make a single pass through each input file? Further, each state's output is probably small enough that all of the data could be generated in memory and then written out when everything is parsed.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

Results

Here' how I reworked the code to implement all of these suggestions:

#include <string>
#include <iostream>
#include <fstream>
#include <unordered_map>
#include <map>
#include <regex>

using StateWeather = std::unordered_map<std::string, std::unordered_map<int, std::map<std::string,int>>>;

std::istream &getWeather (std::istream &in, StateWeather &wx, int year) {
    static const std::regex re{R"x(("([^"]*)"|[^,]*),|$)x"};
    static const std::sregex_token_iterator regend{};
    static constexpr unsigned stateFieldNum{8};
    static constexpr unsigned typeFieldNum{12};
    static constexpr auto maxField{std::max({stateFieldNum, typeFieldNum})};
    std::string line;
    if (std::getline(in, line)) {
        auto it = std::sregex_token_iterator(line.begin(), line.end(), re, 2); 
        std::string state;
        std::string type;
        for (unsigned i=0; i <= maxField && it != regend; ++it, ++i) {
            switch (i) {
                case stateFieldNum:
                    state = *it;
                    break;
                case typeFieldNum:
                    type = *it;
                    ++wx[state][year][type];
                    break;
                default:
                    // do nothing
                    ;
            }
        }
    }
    return in;
}

int main()
{
    static constexpr int startyear{1996};
    static constexpr int endyear{2013};
    StateWeather wx{};
    for (auto year = startyear; year <= endyear; ++year) {
        std::ifstream in{"stormdata_"+std::to_string(year)+".csv"};
        while (getWeather(in, wx, year)) {
        }
    }
    for (auto &statedata : wx) {
        std::ofstream out{statedata.first};
        for (auto year = startyear; year <= endyear; ++year) {
            out << statedata.first << '-' << year << '\n';
            auto &events = statedata.second[year];
            for (const auto &event : events) {
                out << event.first << '\t' << event.second << '\n';
            }
            out << '\n';
        }
    }
}

This code runs on the full data set, generating one file per "state" (the data contains "state" designations such as "GUAM" which is a territory, not a state, and "LAKE ERIE" which is a body of water) in 4.4 seconds on my 64-bit Linux machine. I used gcc version 6.3.1 and -O2 optimization.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.