5
\$\begingroup\$

The program should first check whether all arguments have been specified correctly, and only then execute the actual actions. There should be a waiting period between two actions.

The arguments provided should be parsed using an FSM (if there is no simpler alternative).

Is the error handling correct? Could something be simpler or more robust? I would like to hear what you think about it.

#include <string>
#include <iostream>
#include <map>
#include <vector>
#include <algorithm>
#include <functional>

enum class Symbol
{
    NA,
    SLEEP,
    TARGET,
    ACTION1,
    ACTION2
};

void action1(std::string target, int val)
{
    std::cout << "action1: " << target << ", " << val << std::endl;
}

void action2(std::string target, int val, int magic)
{
    std::cout << "action2: " << target << ", " << val << ", " << magic << std::endl;
}

void action3(int sleep)
{
    std::cout << "action3: " << sleep << std::endl;
}

void exitApp(std::string cause)
{
    std::cout << "EXIT: " << cause << std::endl;
    std::exit(0);
}

/*
 * Call with: ./a.out (sleep=<ms_int>) (target=<ip/host_str>) [a/b]=<port_int> ...
 *   Example: ./a.out sleep=2500 target=localhost b=123 a=23 b=123
 *   Example: ./a.out b=23
 *
 * The order of sleep and target arguments is not important, but they must not be specified twice;
 * the a/b list must contain at least one action and must be at the end.
 */
int main(int argc, char *argv[])
{
    // default values
    std::string target = "0.0.0.0";
    int sleep = 1000;

    std::map<int, std::map<Symbol, int>> fsm;
    fsm[0][Symbol::SLEEP] = 1;
    fsm[0][Symbol::TARGET] = 2;
    fsm[0][Symbol::ACTION1] = 3;
    fsm[0][Symbol::ACTION2] = 3;

    fsm[1][Symbol::TARGET] = 3;
    fsm[1][Symbol::ACTION1] = 3;
    fsm[1][Symbol::ACTION2] = 3;

    fsm[2][Symbol::SLEEP] = 3;
    fsm[2][Symbol::ACTION1] = 3;
    fsm[2][Symbol::ACTION2] = 3;

    fsm[3][Symbol::ACTION1] = 3;
    fsm[3][Symbol::ACTION2] = 3;

    std::vector<std::function<void()>> actions;

    // parse command line
    int i = 1;
    int state = 0;
    while (i < argc)
    {
        std::string arg{argv[i++]};
        size_t equals = arg.find('=');
        if (equals != std::string::npos)
        {
            std::string key = arg.substr(0, equals);
            std::string value = arg.substr(equals + 1);
            Symbol symbol = Symbol::NA;

            if (key == "sleep")
            {
                symbol = Symbol::SLEEP;
            }
            if (key == "target")
            {
                symbol = Symbol::TARGET;
            }
            if (key == "a")
            {
                symbol = Symbol::ACTION1;
            }
            if (key == "b")
            {
                symbol = Symbol::ACTION2;
            }

            if (symbol == Symbol::NA)
            {
                exitApp("Unkown arg");
            }
            if (!fsm[state].count(symbol))
            {
                exitApp("Not allowed arg");
            }

            if (symbol == Symbol::SLEEP)
            {
                sleep = std::stoi(value);
            }
            if (symbol == Symbol::TARGET)
            {
                target = value;
            }
            if (symbol == Symbol::ACTION1)
            {
                int val = std::stoi(value);
                if (val < 1 || val >= 1 << 16)
                {
                    exitApp("Action val not in range");
                }
                actions.push_back([=]()
                                  { action1(target, val); });
                actions.push_back([=]()
                                  { action3(sleep); });
            }
            if (symbol == Symbol::ACTION2)
            {
                int val = std::stoi(value);
                if (val < 1 || val >= 1 << 16)
                {
                    exitApp("Action val not in range");
                }
                actions.push_back([=]()
                                  { action2(target, val, 1234); });
                actions.push_back([=]()
                                  { action3(sleep); });
            }

            state = fsm[state][symbol];
        }
    }
    if (actions.empty())
    {
        exitApp("No action args given");
    }
    actions.pop_back(); // Pop last unneeded action

    // Perfom actions
    for (const auto &action : actions)
    {
        action();
    }

    return 0;
}
\$\endgroup\$
1
  • \$\begingroup\$ Never use std::exit() in C++. It is a C library function. The fact that it is available in C++ does not mean it is for C++. \$\endgroup\$ Commented 6 hours ago

3 Answers 3

Reset to default
5
\$\begingroup\$

Taking a look at the following chunk of code:

    while (i < argc)
    {
        std::string arg{argv[i++]};
        size_t equals = arg.find('=');
        if (equals != std::string::npos)
        {
            std::string key = arg.substr(0, equals);
            std::string value = arg.substr(equals + 1);
            Symbol symbol = Symbol::NA;

            if (key == "sleep")
            {
                symbol = Symbol::SLEEP;
            }
            if (key == "target")
            {
                symbol = Symbol::TARGET;
            }
            if (key == "a")
            {
                symbol = Symbol::ACTION1;
            }
            if (key == "b")
            {
                symbol = Symbol::ACTION2;
            }

            if (symbol == Symbol::NA)
            {
                exitApp("Unkown arg");
            }
            if (!fsm[state].count(symbol))
            {
                exitApp("Not allowed arg");
            }

            if (symbol == Symbol::SLEEP)
            {
                sleep = std::stoi(value);
            }
            if (symbol == Symbol::TARGET)
            {
                target = value;
            }
            if (symbol == Symbol::ACTION1)
            {
                int val = std::stoi(value);
                if (val < 1 || val >= 1 << 16)
                {
                    exitApp("Action val not in range");
                }
                actions.push_back([=]()
                                  { action1(target, val); });
                actions.push_back([=]()
                                  { action3(sleep); });
            }
            if (symbol == Symbol::ACTION2)
            {
                int val = std::stoi(value);
                if (val < 1 || val >= 1 << 16)
                {
                    exitApp("Action val not in range");
                }
                actions.push_back([=]()
                                  { action2(target, val, 1234); });
                actions.push_back([=]()
                                  { action3(sleep); });
            }

            state = fsm[state][symbol];
        }
    }
  • Most of the loop only occurs if = can't be found in the current argument. Rather than indenting all of this, let's skip the argument if it is found.
  • You use std::stoi but don't handle the std::invalid_argument exception it might throw.
  • You probably meant to use std::size_t rather than size_t.
  • Your conditional that sets the value of symbol should use else if to chain the cases together, or alternatively the ternary operator. Or better, this should be shunted out into a function which takes a std::string (or std::string_view) as an argument and returns a Symbol. Maybe this is a member function of the Symbol enum class.
  • Stylistically you probably want a blank line between separate conditionals.
  • Your indentation of pushing back lambdas is a bit odd and unpleasant to read to my eyes. Given the length of those lines it feels entirely extraneous to break these lines up.

Applying some of these suggestions:

    while (i < argc)
    {
        std::string arg{argv[i++]};
        std::size_t equals = arg.find('=');

        if (equals == std::string::npos)
        {
            continue;
        }

        std::string key = arg.substr(0, equals);
        std::string value = arg.substr(equals + 1);
        Symbol symbol = Symbol::NA;

        if (key == "sleep")
        {
            symbol = Symbol::SLEEP;
        }
        else if (key == "target")
        {
            symbol = Symbol::TARGET;
        }
        else if (key == "a")
        {
            symbol = Symbol::ACTION1;
        }
        else if (key == "b")
        {
            symbol = Symbol::ACTION2;
        }

        if (symbol == Symbol::NA)
        {
            exitApp("Unkown arg");
        }

        if (!fsm[state].count(symbol))
        {
            exitApp("Not allowed arg");
        }

        if (symbol == Symbol::SLEEP)
        {
            sleep = std::stoi(value);
        }
        else if (symbol == Symbol::TARGET)
        {
            target = value;
        }
        else if (symbol == Symbol::ACTION1)
        {
            int val = std::stoi(value);
            if (val < 1 || val >= 1 << 16)
            {
                exitApp("Action val not in range");
            }
            actions.push_back([=]() { action1(target, val); });
            actions.push_back([=]() { action3(sleep); });
        }
        else if (symbol == Symbol::ACTION2)
        {
            int val = std::stoi(value);
            if (val < 1 || val >= 1 << 16)
            {
                exitApp("Action val not in range");
            }
            actions.push_back([=]() { action2(target, val, 1234); });
            actions.push_back([=]() { action3(sleep); });
        }

        state = fsm[state][symbol];
    
    }

Other notes:

  • You have functions taking arguments which are never modified, but you don't specify that those arguments are const.
  • exitApp always exits the program with code 0, indicating success, but it seems to be used for errors, which indicates something other than success.
  • Your fsm map never appears to be modified once you set it up, so initialization of it can be simpler and we can make it const.
    std::map<int, std::map<Symbol, int>> fsm;
    fsm[0][Symbol::SLEEP] = 1;
    fsm[0][Symbol::TARGET] = 2;
    fsm[0][Symbol::ACTION1] = 3;
    fsm[0][Symbol::ACTION2] = 3;

    fsm[1][Symbol::TARGET] = 3;
    fsm[1][Symbol::ACTION1] = 3;
    fsm[1][Symbol::ACTION2] = 3;

    fsm[2][Symbol::SLEEP] = 3;
    fsm[2][Symbol::ACTION1] = 3;
    fsm[2][Symbol::ACTION2] = 3;

    fsm[3][Symbol::ACTION1] = 3;
    fsm[3][Symbol::ACTION2] = 3;

Should map (no pun intended) quite nciely to:

    const std::map<int, std::map<Symbol, int>> fsm {
        {0, {{Symbol::SLEEP, 1},
             {Symbol::TARGET, 2},
             {Symbol::ACTION1, 3},
             {Symbol::ACTION2, 3}}},
        {1, {{Symbol::TARGET, 3},
             {Symbol::ACTION1, 3},
             {Symbol::ACTION2, 3}}},
        {2, {{Symbol::SLEEP, 3},
             {Symbol::ACTION1, 3},
             {Symbol::ACTION2, 3}}},
        {3, {{Symbol::ACTION1, 3},
             {Symbol::ACTION2, 3}}}
    };
  • If fsm maps to small integers like 0, 1, 2, and 3, are we sure this shouldn't be std::array<std::map<Symbol, int>, 4>?
  • Or possibly using std::unordered_map?
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your review. You swapped if (symbol == Symbol::NA) and if (!fsm[state].count(symbol)). However, this changes the semantics in an invalid way. Since Symbol::NA is not in the map, "Unknown arg" can never be output. That's why I initially decided against using if-else if-... - even though ... this would obviously be better. And yes, std::unordered_map would be a more suitable choice for the data structure, among others. \$\endgroup\$ Commented 5 hours ago
  • 1
    \$\begingroup\$ Fixed that in an edit. \$\endgroup\$ Commented 5 hours ago
2
\$\begingroup\$

The state change handling:
I fail to follow the flow of the state changes, especially if a state receives an undefined transition (action), like a SLEEP on state 1 or TARGET on state 3.
I assume it works as it should but some comments like "missing transitions will be reported as error during parsing" or "stay in current state".

Separate the argument parsing from the main function:
The main function is quite long and the main content is occupied by the argument parsing code.
I recommend to split the argument parsing at least into two separate functions:

  1. One separate function to validate and parse a symbol string to a Symbol. Separate the representation from the behaviour logic.
  2. Not really needed for the few states and single parameter handling, but if this shall be extensible it can make sense to make a separate parser function for each action instead of having all in a long if-else_if-else block.
  3. Wrap the entire input parameter parsing into actions as one function

Error handling:
The approach with exitApp() is not good.
For one, std::exit() is (at least for me) a brute force way to abort immediately and don't care about any side effects. It is better to log the encountered error and collect an error flag (e.g. a bool) and forward it to skip unwanted operations until you reach the end of main function.
With that you are also able to log multiple errors instead of forcing the caller to fix each problem one after another. This approach also helps to ensure a proper cleanup without the need to put all possible cleanup steps into the existApp() function (e.g. you may want to close some open sockets or a log file or print some common end message or something like this).

The lambdas:
The generation of the lambdas are just mappings to be callable as a void(void) function. I recommend to use std::bind as it is intended for exactly this purpose. This should be more compact and readable than the handwritten lambdas.

State machine states:
Similar as you use an enum class for the state machine transitions (actions), I recommend to make another enumeration for the states istead of the magic numbers 0 to 3. This will increase the readability and helps to prevent misconfigurations, especially completely invalid numbers cannot happen.

\$\endgroup\$
2
\$\begingroup\$

Consider not using a FSM

I personally dislike writing explicit finite state machines. They are hard to understand when small, and nearly impossible when large. It's easy to make mistakes in them. There might be some tools that can generate FSMs for you, or which can verify some properties of a given FSM, but you're not using any of those things either.

Consider that there are only two restrictions you place on the arguments:

  1. sleep and target may only occur at most once
  2. sleep and target are only allowed before any other arguments

I would write that in the following way:

std::optional<std::string> target;
std::optional<int> sleep;
…
switch (symbol) {
case Symbol::SLEEP:
    if (sleep || !actions.empty())
        throw std::runtime_error("'sleep' is only allowed once before actions");
    sleep = std::stoi(value);
    break;

case Symbol::TARGET:
    if (target || !actions.empty())
        throw std::runtime_error("'target' is only allowed once before actions");
    target = value;
    break;

Now you've handled the restrictions of sleep and target. The only difference now is that the default value for those is handled when pushing an action:

case ACTION1: {
    int val = std::stoi(value);
    if (val < 1 || val >= 1 << 16)
        throw std::runtime_error("Action value no in range");
    actions.push_back([=]{ action1(target.value_or("0.0.0.0"), val); });
    actions.push_back([=]{ action3(sleep.value_or(1000)); });
}

case …:
    …
}

You can also consider not using std::optional, but instead use bools to keep track of whether sleep and target have already been used. This allows you to set their default values up front like you do now.

You can reduce some of the code duplication in the code above as well. In any case, it's now more maintainable. Consider what would happen if you added another optional parameter with the FSM: you would need to add one more state, but most states would then also need to handle one more symbol. Basically, your FSM grows as \$O(N^2)\$, where \$N\$ is the number of symbols, whereas "normal" code is often just \$O(N)\$.

Have a look at how command line argument parsing works

Most operating systems use getopt()-style argument parsing on the command line. You could use that for your program as well. Normally programs don't care if optional flags are specified before or after the non-flag arguments. Would it really be a problem if sleep and target are specified after the other arguments? Also, what if you want to specify two actions with different targets on one command line? Consider being able to write:

./a.out --target=localhost a=123 --target=::1 b=456

There might be a good reason why you want the command line argument handling exactly the way you do. But if this isn't a hard requirement, consider making it work more like existing command line applications.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.