1

I'm asked to accept both a number and a unit, where a unit can be cm, m, in or ft.

For this I have a loop as such

cout << "Please put in a number and its unit.\n";
while (cin >> val >> unit) {
    if (val == '|') { break; }
    cout << "Please put in a number and its unit.\n";
}

My question is, what is the best way to check the unit string, when it comes to code efficiency as well as readability? Does it make more sense to put a large if statement

    if (unit != "cm" && unit != "m" && unit != "in" && unit != "ft") {
        cout << "Unit " << unit << " not accepted.\n";
    }

Or is it better to have a vector which I initialise with all the units, and then check if it matches any of the units.

    if (find(units.begin(), units.end(), unit) == units.end()) {
        cout << "Unit " << unit << " not accepted.\n";
    }

... where vector<string> units = {"cm", "m", "in", "ft"};

Or is there another way which is much better in terms of efficiency and readability?

I hope this is the right place to ask this question. I thought about code review but it doesn't seem to fit a small question like this.. or does it?

Tryb Ghost
  • 419
  • 1
  • 4
  • 14
  • 1
    Entirely a matter of taste. "Efficiency" is irrelevant since you are waiting for user input and running this just once each time enter is pressed. – ravenspoint Jun 27 '19 at 15:02

3 Answers3

1

Your "if" statement is not only more efficient, but efficiency doesn't matter because no matter how you do it, the IO will take about a billion times longer.

It's also the most maintainable because if you format the "if" statement as one line per unit string, then adding, removing, or renaming a unit is only a 1-line edit, and you can easily comment them out or in if you change your mind.

The time to use a vector is if you don't know what the units are until run time.

Mike Dunlavey
  • 40,059
  • 14
  • 91
  • 135
1

Does it make more sense to put a large if statement?

The only four entries, I would say that you do not need to use std::vector(and the allocation cost comes up with it) and an algorithm to test the case. If the length of the if the condition is bothering you, pack the condition to a lambda function and call with the user input.

Proper naming of lambda would give much readability than checking the variable directly inside the if statement.

const auto isMatchingUnit = [](const std::string& unit) noexcept -> bool {
    return unit == "cm" || unit == "m" || unit == "in" || unit == "ft";
};

if (isMatchingUnit(unit)) {
    // do something
} else {
    // do something else
}

That being said, if the number of possible input s are large, you might wanna pack the possible inputs to std::arrayor std::vector and apply the standard algorithms as per choice:

If the collection to be checked is sorted and unique you can

  • std::binary_search which returns true, if item is found.
  • std::lower_bound by checking std::lower_bound(container.cbegin(), container.cend(), input) != container.cend(), for true case.

If the collection to be checked is not sorted

  • std::find by checking std::find(container.cbegin(), container.cend(), input) != container.cend(), for true case.
  • std::any_of with a proper predicate.
JeJo
  • 30,635
  • 6
  • 49
  • 88
  • What "allocation cost" with `std::array`? – Lightness Races in Orbit Jun 27 '19 at 14:59
  • @LightnessRacesinOrbit I meant about `std::vector`. will edit! – JeJo Jun 27 '19 at 14:59
  • Why a lambda? Good case for a normal function, this. – Lightness Races in Orbit Jun 27 '19 at 14:59
  • @LightnessRacesinOrbit I hope you know [this](https://stackoverflow.com/questions/13722426/why-can-lambdas-be-better-optimized-by-the-compiler-than-plain-functions) – JeJo Jun 27 '19 at 15:02
  • There are no function pointers or anything of that kind here. Don't see the relevance – Lightness Races in Orbit Jun 27 '19 at 15:24
  • @LightnessRacesinOrbit What if the function(here lambda) needs to be called once. Do you prefer the definition next to where it's been called or somewhere else in the translation unit? especially for a function which has a single return statement as a body. – JeJo Jun 27 '19 at 15:28
  • If it's a utility that only makes any semantic sense within the lexical scope of that function, a lambda. If it's a more general piece of logic, as it appears to be here, I'd prefer a proper function elsewhere in the TU where it's easy to spot & maintain (even if currently only used once). But that's definitely subjective :) – Lightness Races in Orbit Jun 27 '19 at 15:35
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/195643/discussion-between-jejo-and-lightness-races-in-orbit). – JeJo Jun 27 '19 at 15:53
0

The most efficient would be using std::lower_bound on a sorted array of strings, so it doesn't search all of them. If you use direct comparison or std::find, then all will be searched.

For a small vector, it might not be important (it might even be slower than a direct comparison), but for larger sets it will.

Michael Chourdakis
  • 10,345
  • 3
  • 42
  • 78