Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

static const std::map<string, int> vs if-elseif

I write a function that should convert a string to a number. I see two possible variants to write it:

int convert(const std::string input) {
    if (input == "one") {
        return 1;
    } else if (input == "two") {
        return 2;
    }
    // etc.
    return 0;
}

Or

int convert(const std::string input) {
    static const map<string, int> table = {
        {"one", 1},
        {"two", 2}
        // etc.
    }

    const auto result = table.find(input);

    if (result == table.end())
    {
        return 0;
    }

    return result->second;
}

What way is more effective/acceptable/readable?

like image 839
Alex Avatar asked May 03 '16 15:05

Alex


5 Answers

The answer greatly depends on how many different strings you are going to support by this.

A few strings: go with if-else. The effort needed to understand the code later is little.

A lot of strings: Create a map. The effort understanding the code is small compared to the effort reading a huge if-else construct. Probably, you will have to extend this list often. Adding data requires less typing.

I am not sure how smart C++'s map is using strings as keys. In the worst case, both have the same performance. If the list gets really huge, you might think of creating a hash value of the strings and use this as a key. This might improve performance greatly. You will have to make sure that collisions don't happen though. (A good hash algorithm and 64 bit hash size should be sufficient.) It might be that modern map implementations do this already.

like image 96
philipp Avatar answered Nov 08 '22 18:11

philipp


For small set of text, I'd use a simple lookup table:

struct LookupTable {
    const char* text;
    int value;
};
const LookupTable table[] = {
    { "one", 1 },
    { "two", 2 }
};
int convert(const char* text) {
    if (!text) return 0;
    for (int i=0; i<sizeof(table)/sizeof(LookupTable); i++) {
        if (strcasecmp(text, table[i].text) == 0 ) {
            return table[i].value;
        }
    }
    return 0;
}

For large set of text, I would consider using std::unordered_map<std::string,int>, and perhaps custom hash function (bkdr hash or elf hash is good on words).


EDIT: As David pointed out in comment, if you don't want the ugly sizeof, use the modern for-loop:

int convert(const char* text) {
    if (!text) return 0;
    for (auto& entry: table) {
        if (strcasecmp(text, entry.text) == 0 ) {
            return entry.value;
        }
    }
    return 0;
}
like image 36
Non-maskable Interrupt Avatar answered Nov 08 '22 19:11

Non-maskable Interrupt


An if-else (or a switch, if available to you) are good for small cases, and you can also control the order of the tests in case the most common tests can cut out the search quickly, you can test them first.

In many cases, a switch is far better than a list of if-elses. Both easier to read and very possibly faster. Though switch is not the best choice with string.

You could however switch on an enum rather than using strings; this is certainly the better approach, except for a map.

A map or std::unordered_map is far better for large numbers of possibilities or when you need these possibilities updated at runtime.

like image 6
johnbakers Avatar answered Nov 08 '22 19:11

johnbakers


For a small number of possible input values, I would prefer solution 1 which is straightforward and probably has the best performance.

If the list of values becomes too big, then what you really need is a converter between integers and written numbers, and that really is a different story (see the "Humanizer" library referenced in the comment by NathanOliver

like image 3
GdR Avatar answered Nov 08 '22 19:11

GdR


What way is more effective/acceptable/readable?

The if/else solution is the most efficient if you only have a couple values, and is certainly quite straightforward especially for people not used to the Standard Library, however it quickly devolves into a mess.

Thus, as soon as you reach a 5 or more items, switch to using a container.

Caveat: unfortunately, std::string_view, which would avoid a memory allocation, is still not standard; for simplicity I will thus use std::string, though if memory allocation is an issue, std::string_view or a custom CStr class would be better.

There are 3 valid choices:

  • std::map<std::string, int> and std::unordered_map<std::string, int> are the most intuitive choices, it's unclear which would be faster
  • std::vector<std::pair<std::string, int>> (sorted) will always be more efficient than std::map<std::string, int>

Thus, if efficiency is a concern:

int convert(std::string const& name) {
    static std::vector<std::pair<std::string, int>> const Table = []() {
        std::vector<std::pair<std::string, int>> result = {
            { "one", 1 },
            { "two", 2 },
            { "three", 3 },
            { "four", 4 }
        };
        std::sort(result.begin(), result.end());
        return result;
    }();

    auto const it =
        std::lower_bound(Table.begin(), Table.end(), std::make_pair(name, 0));

    if (it != Table.end() and it->first == name) {
        return it->second;
    }
    return 0;
}

A sorted array is, after all, the most efficient way to perform a binary search, because of a better cache behavior. It should also outperform std::unordered_map on smallish inputs for the same reasons.

Of course, it is slightly less readable.

like image 3
Matthieu M. Avatar answered Nov 08 '22 20:11

Matthieu M.