How could I improve this code ( C++ )

Question

Hello !

I want your suggestion on the following pseudo-code . Please suggest how could I improve it,whether or not I could use some design patterns .

// i'm receiving a string containing : id operation arguments
data    = read(socket);
tokens  = tokenize(data," "); // tokenize the string based on spaces
if(tokens[0] == "A") {
   if(tokens[1] == "some_operation") {
      // here goes code for some_operation , will use the remaining tokens as arguments for function calls
   }
   else if(tokens[1] == "some_other_operation") {
     // here goes code for some_other_operation , will use the remaining tokens
   }
   ...
   else {
     // unknown operation
   }
}
else if(tokens[0] == "B") {
   if(tokens[1] == "some_operation_for_B") {
     // do some operation for B
   }
   else if(tokens[1] == "yet_another_operation") {
     // do yet_another_operation for B
   }
   ...
   else {
     // unknown operation
   } 
}

I hope you get the point . The thing is I have a large number of id's and each has it's own operations , and I think it's kinda ugly to have 10 screens of code containing a lot of if's and else if's.

Answer 1

Have a class for each ID which implements a common interface. Basically the Strategy pattern IIRC.

So you'd call (pseudo)code like:

StrategyFactory.GetStrategy(tokens[0]).parse(tokens[1..n])

Answer 2

You want to split this up into multiple functions, one for each ID, and one for each operation.

A guideline I normally use is a screen height. If I can't have a function in full fit on my screen I start thinking about splitting things up. That way you don't need to scroll just to see where the function is going. As I said, it's a guideline, not a rule, but I find it more practical to be in control of the structure.

If you then want to take an OO approach and turn this into a bunch of classes, you're welcome to do so if you see an advantage. Be mindful of all the plumbing that goes with it, though. You might want to keep it simple.

Dave

Answer 3

Create a map of functions. Then you'd have code like:

consumed_count = token_mapper[tokens[0]](tokens)
remove amount of consumed tokens according to the return value and repeat.

Though, I don't understand your approach anyway, You are going to write a language that's hard to handle and inflexible. Think about it: A small difference in the amount of arguments causes real havoc in that language. Therefore you are always limited to 1-3 arguments per command.

I'd rather just use some lexer/parser generator combination, but if you want to do what you are going to do, I'd propose you at least split first with newline, then with space, and have therefore clear way to see whether it was meant to give 2 or 3 arguments.

It's important even if your language would be machine-generated, what if your generator ends up having a bug? Fail early, fail often.

Answer 4

You could have a look into "Table Driven Methods" (as described in "Code Complete", 2nd edition, chapter 18). I think this is what Cheery describes. A benefit of that is easy extensibility. You just have to add some entries to the table. This table could be hard coded or even loaded at run-time.

Similar to Epaga's suggestion you coul also try to solve this via polymorphism, having specialized classes perform actions for the different cases. The drawback here is that you have to write new classes in case of changes.

Answer 5

you could use the command pattern...... each of your actions would know its id and operation and add itself to to a list at run time...then you'd simply look up the right command, pass it whatever context it needs and it will execute the operation.

Answer 6

The table driven aproach seems to fit this, like mxp said. If you have diffrent number of parameters for your functions you could have a column in the table that specifies the number of parameters for the function on the same row.

Answer 7

First write down the syntax of what you support, then write the code to support it.

Using BNF notation is great for that. And using the Spirit library for the code-part is quite straightforward.

Command := ACommand | BCommand

ACommand := 'A' AOperation
AOperation := 'some_operation' | 'some_other_operation'

BCommand := 'B' BOperation
BOperation := 'some_operation_for_B' | 'some_other_operation_for_B'

This easily translates into a Spirit parser. Every production rule would become a one-liner, every end-symbol would be translated into a function.

#include "stdafx.h"
#include <boost/spirit/core.hpp>
#include <iostream>
#include <string>

using namespace std;
using namespace boost::spirit;

namespace {
    void    AOperation(char const*, char const*)    { cout << "AOperation\n"; }
    void    AOtherOperation(char const*, char const*)    { cout << "AOtherOperation\n"; }

    void    BOperation(char const*, char const*)    { cout << "BOperation\n"; }
    void    BOtherOperation(char const*, char const*)    { cout << "BOtherOperation\n"; }
}

struct arguments : public grammar<arguments>
{
    template <typename ScannerT>
    struct definition
    {
        definition(arguments const& /*self*/)
        {
            command
                =   acommand | bcommand;

            acommand = chlit<char>('A') 
              >> ( a_someoperation | a_someotheroperation );

            a_someoperation = str_p( "some_operation" )           [ &AOperation ];
            a_someotheroperation = str_p( "some_other_operation" )[ &AOtherOperation ];

            bcommand = chlit<char>('B') 
              >> ( b_someoperation | b_someotheroperation );

            b_someoperation = str_p( "some_operation_for_B" )           [ &BOperation ];
            b_someotheroperation = str_p( "some_other_operation_for_B" )[ &BOtherOperation ];

        }

        rule<ScannerT> command;
        rule<ScannerT> acommand, bcommand;
        rule<ScannerT> a_someoperation, a_someotheroperation;
        rule<ScannerT> b_someoperation, b_someotheroperation;

        rule<ScannerT> const&
        start() const { return command; }
    };
};

template<typename parse_info >
bool test( parse_info pi ) {
  if( pi.full ) { 
    cout << "success" << endl; 
    return true;
  } else { 
    cout << "fail" << endl; 
    return false;
  }
}

int _tmain(int argc, _TCHAR* argv[])
{

  arguments args;
  test( parse( "A some_operation", args, space_p ) );
  test( parse( "A some_other_operation", args, space_p ) );
  test( parse( "B some_operation_for_B", args, space_p ) );
  test( parse( "B some_other_operation_for_B", args, space_p ) );
  test( parse( "A some_other_operation_for_B", args, space_p ) );

    return 0;
}

Answer 8

I've seen a solution to this problem that worked well: a hash table of functions.

At compile time a Perfect Hash Function is created for each supported operation and the operation is associated with a function to call (the function pointer is the value in the hash, the command string is the key).

During runtime, the command functionality is called by using the command string to find the function in the hash table. Then the function is called passing the "data" string by reference. Each command function then parses out the remaining string according to its rules... the strategy pattern also applies at this point.

Makes the code work like a state machine, which is (IMHO) the easiest way to approach networking code.