0

I want to simplify this code and want to make it smarter, there are about 80 singleS and multipleS's. I considered State design pattern usage but it seems that it wont be simpler. Can anybody help me with the below lines:

void spy::run(string num, SingleS single, MultipleS multipleS)
{
if(num == "1")
{singleS.runS1}
else if(num == "2")
{singleS.runS2}
else if(num == "3")
{singleS.runS3}
else if(num == "4")
{singleS.runS4}
else if(num == "5")
{singleS.runS5}
else if(num == "6")
{singleS.runS6}
else if(num == "7")
{singleS.runS7}
else if(num == "8")
{singleS.runS8}
else if(num == "9")
{singleS.runS9}
else if(num == "10")
{multipleS.runS10}
else if(num == "11")
{multipleS.runS11}
else if(num == "12")
{multipleS.runS12}
else if(num == "13")
{multipleS.runS13}
}
}
Martijn Pieters
  • 1,048,767
  • 296
  • 4,058
  • 3,343
  • 2
    First of all I would recommend that you convert `nun` to an actual integer number. Then you could easily use `num - 1` (for "singles") or `num - 10` (for "multiples") as an index into a couple of arrays of functions pointers. Of you could use a `std::map` (or `std::unordered_map`) that maps the strings (or numbers) to the function to call. – Some programmer dude Jun 03 '20 at 07:23
  • [This question](https://stackoverflow.com/questions/650162/why-the-switch-statement-cannot-be-applied-on-strings) and the answers can probably help you. – user1810087 Jun 03 '20 at 07:33

2 Answers2

4

You can use std::map or std::unordered_map like that

std::unordered_map<std::string, std::function<void(void)>> functionsMap = {
   {"1", std::bind(&SingleS::runS1, &singleS)},
   {"12", std::bind(&MultipleS::runS12, &multipleS)},
...
};

auto it = functionsMap.find(num);
if (it != functionsMap.end())
    it->second();
...

Links: http://www.cplusplus.com/reference/functional/bind/, http://www.cplusplus.com/reference/functional/function/function/, http://www.cplusplus.com/reference/unordered_map/unordered_map/

Skident
  • 326
  • 1
  • 5
  • Can you a bit explain how you code like that. Is it about functors, daoes it require c++11 and later versions ? –  Jun 03 '20 at 10:17
  • Yes, this code requires C++11 or higher. It's also possible to do the same code on older versions of C++ if needed. `std::function` - any function which returns void and doesn't take arguments is allowed (lambdas can be used as well). `std::bind(&SingleS::runS1, &singleS)` - creates a pointer to a function by binding the object and the method `std::unordered_map` - a container which does search for a constant time O(1), where the key is quite any type which can be hashed (standard types like: int, string are already supported). – Skident Jun 03 '20 at 10:40
  • There is a problem with the code, cannot convert from brace enclosed initializer list to std::unordered_map –  Jun 03 '20 at 11:02
  • Actually fuction has a requirements parameter. This might be the cause i but i have to figure out hoq implement parameter –  Jun 03 '20 at 11:09
  • do all your functions take the same parameter type? you should set that type in your std::function declaration and also update a `bind` function, something like that `std::function` `std::bind(&SingleS::runS1, & singled, std::placeholder::_1, std::placeholder::_2);` – Skident Jun 03 '20 at 11:21
-2

You can try nested ternary :

void spy::run(string num, SingleS single, MultipleS multipleS)
{

    (num=="1")?singleS.runS1:(num=="2")?singleS.runS2:(num=="3")?singleS.runS3:(num=="4")...;
}
rekkalmd
  • 171
  • 1
  • 12
  • 3
    That's even worse than having a bunch of `if/elseif`. by the way, `num` is a string, not a char – Cid Jun 03 '20 at 08:36
  • @Cid Yes in fact ! A question btw, why would it be worse ? – rekkalmd Jun 03 '20 at 08:40
  • Because it will become unreadable very quickly and hard to maintain. Just write the whole ternary and you'll understand why. You're just doing exactly the same than in the question, but on one looong line – Cid Jun 03 '20 at 08:41
  • @Cid -- with appropriate indentation nested ternary operators can be quite readable. As a one-liner, obviously, it's disastrous. But an `if ... else if ...` ladder all on one line would be, too. – Pete Becker Jun 03 '20 at 12:12
  • I can join you for the lack of some salt in this one, which is secondary, but "Hard to maintain", can you develop, please ? – rekkalmd Jun 03 '20 at 12:38
  • 1
    @rekkalmd sure, by hard to maintain, I point the readability, but as stated Pete, nested ternaries can be indented but in that case it worth using instead if/else if (or even better, [Skident's answer](https://stackoverflow.com/a/62167378/8398549)), since that ternary doesn't return anything. If someday you have to work on a piece of code that contains 13 nested ternaries, you'll curse the developper who wrote it. – Cid Jun 03 '20 at 12:50