2
\$\begingroup\$

I am simulating a bridge where only one car can cross at a time. The bridge runs north south. The car arrival times and times taken to cross the bridge are given in an input file. The format is direction|arrival time,time taken to cross. The possible directions are north and south, represented by N/n and S/s respectively. An upper case letter means high priority and a lower case letter means low priority. The arrival times and time to cross are can have a value after the decimal e.g. 10.5

Examples:

N|6.0,10.0

n|4.5,5.5

Here is my function, which is pretty ugly but works:

void getInput(char * inFile)
{
    enum direction_t {NORTH, SOUTH};
    direction_t direction;
    enum priority_t {HIGH, LOW};
    priority_t priority;
    string line;
    ifstream inFile(inFile);
    if(!inFile.good())
    {
        cerr << "can't open input file";
        exit(1);
    }
    while(getline(inFile, line))
    {
        char first = line.at(0);//first letter signifies direction and priority
        if(first == 'n')
        {
            direction = NORTH;
            priority = LOW;
        }
        else if(first == 'N')
        {
            direction = NORTH;
            priority = HIGH;
        }
        else if(first == 's')
        {
            direction = SOUTH;
            priority = LOW;
        }
        else if(first == 'S')
        {
            direction = SOUTH;
            priority = HIGH;
        }
        else
        {
            cerr << "invalid direction given: " << first << endl;
            //exit(1);
        }
        size_t commaIndex = line.find(",", 2);//know first character is letter, second is |
        string firstNumber = line.substr(2, commaIndex-2);
        string secondNumber = line.substr(commaIndex+1, line.length());
        cout << "firstNumber: " << firstNumber << " secondNumber " << secondNumber << endl;   
    }

I was wondering if the way I use enum is good. Any tips for making code more readable and more compact are appriaciated.

\$\endgroup\$
4
\$\begingroup\$

Parameter shadowing a local variable:

The first issue with your code that prevented in from compiling with Clang is the inFile parameter of getInput() that shadows the ifstream instance of the same name.

Your compiler seems to be more forgiving with that problem. If you are using GCC, I recommend using the -Wshadow warning flag to enable warnings of this kind of variable name shadowing/redeclaration. This category of bugs can be particularly hard to track in a scenario where both variables are of the same type.

Reserved suffix:

The _t suffix is reserved by the POSIX standard (read this), so it is best if avoided. Consider renaming direction_t and priority_t to Direction and Priority.

Const correctness:

The filename parameter of the function is a read-only string, so it should at least be a const char *. But since this is C++ code, prefer a standard string and pass it by const reference:

void getInput(const std::string & filename)

Error handling:

You did right by using std::cerr to report errors, however, exit()ing after the error is logged seems like a very crude way of handling it. Consider throwing an exception or even making the function return a boolean false.

Using namespace:

You are probably using namespace std somewhere, judging by the lack of the std:: prefix in the library calls. This is best if avoided as it defeats the purpose of a namespace, which is allowing equal names to coexist without clashes. Read this thread for a more in-depth discussion.

Data driven solution:

There is a more data driven and flexible way of handling the conversion between the chars read from file to the Direction/Priority enums than an if chain or a switch statement.

You could instead use a map keyed by a char. This way, expanding and adding more chars is simpler and involves less code change. You could even externalize the map to a file and load it dynamically at runtime if you wanted.

Following is the refactored code with the changes above mentioned applied to it plus the use of a std::map as a lookup table. The map associates a char (the key) with a pair of [Direction, Priority]. Making the conversion form the file data a lot more concise.

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

bool getInput(const std::string & filename)
{
    enum Direction { NORTH, SOUTH };
    enum Priority  { HIGH,  LOW   };

    const std::map<char, std::pair<Direction, Priority>> lookupTable = {
        { 'n', { NORTH, LOW  } },
        { 'N', { NORTH, HIGH } },
        { 's', { SOUTH, LOW  } },
        { 'S', { SOUTH, HIGH } }
    };

    Priority    priority;
    Direction   direction;
    std::string line;

    std::ifstream inFile(filename);
    if (!inFile.good())
    {
        std::cerr << "can't open input file";
        return false;
    }

    while (getline(inFile, line))
    {
        const char key = line.at(0); // first letter signifies direction and priority

        const auto entry = lookupTable.find(key);
        if (entry != lookupTable.end())
        {
            direction = entry->second.first;
            priority  = entry->second.second;
        }
        else
        {
            std::cerr << "invalid direction given: " << key << std::endl;
        }

        size_t commaIndex = line.find(",", 2); // know first character is letter, second is |
        std::string firstNumber  = line.substr(2, commaIndex - 2);
        std::string secondNumber = line.substr(commaIndex + 1, line.length());
        std::cout << "firstNumber: " << firstNumber << " secondNumber " << secondNumber << std::endl;
    }

    return true;
}

So it turn out that the OP's compiler is missing a few C++11 features that prevent the code provided above from compiling. It can be fairly easily adapted to support older compilers though, if we are willing to add a few not so elegant workarounds. Here is one way to adapt it using an aux function that keeps a static std::map:

enum Direction { NORTH, SOUTH, DIR_INVALID };
enum Priority  { HIGH,  LOW,   PRI_INVALID };

std::pair<Direction, Priority> lookup(char key)
{
    typedef std::map< char, std::pair<Direction, Priority> > TableType;

    static bool isTableInitialized = false;
    static TableType lookupTable;

    // One time initialization
    if (!isTableInitialized)
    {
        lookupTable.insert(std::make_pair( 'n', std::make_pair(NORTH, LOW)  ));
        lookupTable.insert(std::make_pair( 'N', std::make_pair(NORTH, HIGH) ));
        lookupTable.insert(std::make_pair( 's', std::make_pair(SOUTH, LOW)  ));
        lookupTable.insert(std::make_pair( 'S', std::make_pair(SOUTH, HIGH) ));
        isTableInitialized = true;
    }

    TableType::const_iterator entry = lookupTable.find(key);
    if (entry != lookupTable.end())
    {
        return (*entry).second;
    }

    // Not found / invalid key
    return std::make_pair(DIR_INVALID, PRI_INVALID);
}

bool getInput(const std::string & filename)
{
    Priority    priority;
    Direction   direction;
    std::string line;

    std::ifstream inFile(filename.c_str());
    if (!inFile.good())
    {
        std::cerr << "can't open input file";
        return false;
    }

    while (getline(inFile, line))
    {
        // first letter signifies direction and priority
        const char key = line.at(0);

        // Lookup the conversion table:
        const std::pair<Direction, Priority> entry = lookup(key);

        if ((entry.first != DIR_INVALID) && (entry.second != PRI_INVALID))
        {
            direction = entry.first;
            priority  = entry.second;
        }
        else
        {
            std::cerr << "invalid direction given: " << key << std::endl;
        }

        size_t commaIndex = line.find(",", 2); // know first character is letter, second is |
        std::string firstNumber  = line.substr(2, commaIndex - 2);
        std::string secondNumber = line.substr(commaIndex + 1, line.length());
        std::cout << "firstNumber: " << firstNumber << " secondNumber " << secondNumber << std::endl;
    }

    return true;
}

It has its drawback, but should work just the same. It is also quite a bit more code to get it done. One important thing to keep in mind is that initialization order of the lookup table is not thread safe, so don't try to use this in a concurrent program.

\$\endgroup\$
  • 1
    \$\begingroup\$ Good points. I'd also suggest creating a VehicleArrival class to contain this data and then adding a std::istream &operator>> method, encapsulating it in a neat, object-oriented way, rather than having an ugly mix of input and output in one function. \$\endgroup\$ – Edward Oct 24 '14 at 14:26
  • \$\begingroup\$ @Edward - Indeed, that would look much nicer! But I'm leaving that one up to you ;) \$\endgroup\$ – glampert Oct 24 '14 at 14:29
  • 1
    \$\begingroup\$ Just coded one and posted it. \$\endgroup\$ – Edward Oct 24 '14 at 15:59
  • \$\begingroup\$ const std::map<char, std::pair<Direction, Priority>> lookupTable = { produces a whole ton of errors when compiling. \$\endgroup\$ – Celeritas Oct 25 '14 at 6:30
  • 1
    \$\begingroup\$ +1 for comment about not using _t in identifiers. I red the article linked to, so the idea is if it's reserved for something else (such as POSIX) I shouldn't use it, right (unless I'm working for POSIX =) ) \$\endgroup\$ – Celeritas Oct 25 '14 at 9:21
3
\$\begingroup\$

Since the other reviews hit most of the important things, I thought I'd just show one alternative that might be useful to you. First, since each line of input represents a vehicle arrival, let's create a VehicleArrival class and have it encode and decode to and from streams.

VehicleArrival class

class VehicleArrival
{
private:
    bool northbound;
    bool priority;
    float time;
    float duration;

The data portion is fairly simple, so now for the interesting pieces. First, some private helper functions for input and output:

    void dirdecode(char ch) { 
        priority = std::isupper(ch);
        northbound = std::tolower(ch) == 'n';
    }
    char direncode() const {
        char ch = northbound ? 'n' : 's';
        if (priority) 
            ch = std::toupper(ch);
        return ch;
    }

The purpose for these should be fairly obvious, but I should point out that the dirdecode is very generous about its interpretaton of "south" because anything that isn't 'n' or 'N' is interpreted as "south." If you cared to, you could change that function to be more strict, and perhaps pass a bool back indicating success or failure.

Typically, the inserter is usually easier to write than the extractor, so I usually write it first:

    friend std::ostream &operator<<(std::ostream &out, const VehicleArrival &va) 
    {
        char ch = va.direncode();
        return out << ch << '|' << std::fixed << std::setprecision(1) 
                   << va.time << ',' << va.duration;
    }

This isn't great code because I've hard-coded the separator char '|' and the ','. In particular, the latter might not be very smart if, for example, the ostream is using, say, a German locale where a comma is used as the decimal point. However, having noted that issue, I'll leave it for you to resolve.

Lastly we have the extractor code:

    friend std::istream &operator>>(std::istream &in, VehicleArrival &va) 
    {
        char ch, sep, comma;
        in >> ch >> sep >> va.time >> comma >> va.duration;
        if (sep != '|' || comma != ',')
            in.setstate(std::ios_base::failbit);
        va.dirdecode(ch);
        return in;
    }
};

There are a couple of important things to note here. First, note again that the separator and comma are hardcoded. In real code, one would actually use a static const member or perhaps a plain old data member to allow these characters to be changed by the user of the class. Again, I'll leave that to you.

The other important point here is the setting of std::ios_base::failbit. This flag is implicitly set if, say, the float extractions fail, but this causes that bit to be explicitly set if the formatting was wrong. This is handy because this way you don't have to write your own exception code. Instead, you can simply use the exception mechanism already built into std::istream. More on that in a bit.

Driver code

If you'll pardon the pun, here's driver code to exercise the functions in the class:

#include <iostream>
#include <iomanip>
#include <cctype>
#include <vector>

// the VehicleArrival class code goes here

int main()
{
    VehicleArrival va;
    // std::cin.exceptions(std::istream::failbit);
    std::vector<VehicleArrival> cars;
    while (std::cin >> va)
        cars.push_back(va);

    for (auto const &car : cars)
        std::cout << car << '\n';
}

As you can see, using this code is really simple. The only thing I wanted to point out is the commented out line.

    // std::cin.exceptions(std::istream::failbit);

If that line is uncommented, formatting errors in the input line will cause an exception. For example, when I feed this program bad data on my Linux machine with that line uncommented, I get this result

terminate called after throwing an instance of 'std::ios_base::failure'
what():  basic_ios::clear
Aborted (core dumped)

Obviously, you'd use try and catch in real code, but this is useful for demonstrating the technique.

Sorry that's so long, but I think it's a very useful technique and deserves a full explanation.

\$\endgroup\$
  • \$\begingroup\$ Very good examples and well explained. +1 \$\endgroup\$ – glampert Oct 24 '14 at 16:39
  • \$\begingroup\$ Hi Edward. Is there any reason you chose to make VehichleArrival class a class and not a struct? \$\endgroup\$ – Celeritas Oct 25 '14 at 9:28
  • \$\begingroup\$ It's a class rather than a struct because generally one would want to encapsulate the data privately rather than exposing it to the world. With that said, though, this particular class is pretty useless because I didn't provide any accessor functions other than the stream I/O. For a real program, I would probably prefer to write some member access functions (and probably some constructors) rather than making it a struct. It depends on the needs of the program. \$\endgroup\$ – Edward Oct 25 '14 at 11:44
2
\$\begingroup\$

I was confused by your naming of inFile for a little while and was wondering why you were passing something to itself to initialize itself, until I noticed that you had inFile as a char* as well as ifstream. I'd name the char* something like fileName (I assume that's what it is?).

I'd possibly re-order the code that you have at the top of the function to make it a little more readable.

enum direction_t {NORTH, SOUTH};
enum priority_t {HIGH, LOW};

direction_t direction;
priority_t priority;

It's personal preference, but I prefer the definition of the eums to be grouped together. More ideally, I'd put them in a .h file as opposed to in a function definition.

For your first section of actual logic, you seem to have quite a few if else if... statements, which you could probably clean up into a switch/case; something along the lines of:

switch(first)
{
    case 'n':
        direction = NORTH;
        priority = LOW;
        break;
    case 'N':
        direction = NORTH;
        priority = HIGH;
        break;
    case 's':
        direction = SOUTH;
        priority = LOW;
        break;
    case 'S':
        direction = SOUTH;
        priority = HIGH;
        break;
    default:
        cerr << "Invalid direction given: " << first << endl;
        break;
}

You've named your strings at the bottom of the function firstNumber and secondNumber. I'd find these more descriptive if they were called something better to reflect what they are - arrTime, timeTaken or something like that.

As well as all of that, I imagine that you have the line using namespace std; somewhere? Don't, for these reasons.

\$\endgroup\$
  • \$\begingroup\$ I thought in C++ switch statments only worked on integers, not chars? \$\endgroup\$ – Celeritas Oct 24 '14 at 8:58
  • \$\begingroup\$ @Celeritas I'm pretty sure that it should work with chars, as they're unsigned ints with some stuff added. I'll double check. \$\endgroup\$ – Yann Oct 24 '14 at 9:00
  • \$\begingroup\$ @Celeritas Yup, works on my machine \$\endgroup\$ – Yann Oct 24 '14 at 9:02
  • \$\begingroup\$ From here "The expression used in a switch statement must have an integral or enumerated type, or be of a class type in which the class has a single conversion function to an integral or enumerated type." but what does mean an "integral" type? \$\endgroup\$ – Celeritas Oct 24 '14 at 9:10
  • \$\begingroup\$ @Celeritas It's whether it relates to an integer, or is derived from int in some way. All of these types are of integral type. \$\endgroup\$ – Yann Oct 24 '14 at 9:13

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.