Simple server to manipulate priority queues

Question

In a few months I'll work on a C++ project that could span a year or two. I've a strong C background and for the usual reasons I always chose C over C++ (please forgive me). To extend my capabilities and to move from C to some more powerful language, I felt the time has come to take a serious step towards C++. In order to do so I have created a small C++ project just for myself. I often read that C programmers do not use the power C++ and all of its glory has to offer, something I madly tried to avoid. I would like some code reflection, points of improvement and other tips. Any suggestions or enhancements are welcome.

Please note that the code below is only to illustrate my C++ programming skills. Although the project will compile (and run) is has no purpose, neither does it really do anything. Just a learning experience...

#include <iostream>
#include <list>
#include <stdlib.h>
#include <string.h>
#include <vector>
#include <iterator>
#include <stdexcept>

#include "queue.h"

using namespace std;

static list<Queue> listQueue;

void tokenizeCmd(const string &str, vector<string> &tokens, const string &delimiters = " ") {
    string::size_type lastPos = str.find_first_not_of(delimiters, 0);
    string::size_type pos = str.find_first_of(delimiters, lastPos);

    while (string::npos != pos || string::npos != lastPos) {
        tokens.push_back(str.substr(lastPos, pos - lastPos));
        lastPos = str.find_first_not_of(delimiters, pos);
        pos = str.find_first_of(delimiters, lastPos);
    }
}

void processCommand(string strCmd) {
    vector<string> tokens;
    tokenizeCmd(strCmd, tokens);

    if (tokens.empty())
      return;

    try {
        if(tokens.at(0) == "list") {
            cout << "All queues [" << listQueue.size() << "]" << endl;
            for(std::list<Queue>::iterator it = listQueue.begin(); it != listQueue.end(); ++it)
                std::cout << it->GetName() << "\t\tProirity: " << it->GetPriority() << '\n';

        } else if(tokens.at(0) == "add") {
            Queue cA(tokens.at(1));
            if (tokens.size() > 2)
                cA.SetPriority(std::stoi(tokens.at(2)));
            listQueue.push_back(cA);

        } else if(tokens.at(0) == "remove") {
            for(std::list<Queue>::iterator it = listQueue.begin(); it != listQueue.end(); ++it)
                if(it->GetName() == tokens.at(1))
                    listQueue.erase(it++);

        } else if(tokens.at(0) == "update") {
            if (tokens.size() < 3)
                return;

            try {
                for(std::list<Queue>::iterator it = listQueue.begin(); it != listQueue.end(); ++it)
                    if(it->GetName() == tokens.at(1))
                        it->SetPriority(std::stoi(tokens.at(2)));
            } catch (const std::exception &ex) {
                std::cerr << ex.what() << '\n';
            }

        } else if(tokens.at(0) == "help") {
            cout << "Available commands:\n"
                 << "\tadd <name> [priority]\n"
                 << "\tremove <name>\n"
                 << "\tupdate <name> <priority>\n"
                 << "\tlist\n";
        } else if(tokens.at(0) == "quit") {
            exit(0);
        }
    } catch (const std::out_of_range &oor) {
        std::cerr << "Too few arguments for " << tokens.at(0) << '\n';
    }
}

void shell() {
    string strCmd;

    cout << "Switch to interactive shell\n";
    cout << "qsh> ";
    while(std::getline(std::cin, strCmd)) {
      processCommand(strCmd);
      cout << "qsh> ";
    }
}

void usage(char *pchProgram) {
    cout << "Usage: " << pchProgram << '\n'
         << " -h\tShow help\n"
         << " -d\tDaemonize the server (default)\n"
         << " -s\tStart interactive shell\n"
         << " -f\tRun on foreground\n";
}

void initialize() {
    Queue cGeneral("generic");
    cGeneral.SetSystemFlag(true);
    listQueue.push_back(cGeneral);
}

int main(int argc, char **argv) {

    /* Create at least one queue */
    initialize();

    if (argc < 2)
        /*TODO daemon */
        cout << "Starting server\n";
    else
        for(int i=1; i<argc; i++)
            if (!strcmp(argv[i], "-s"))
                shell();
            else if (!strcmp(argv[i], "-q"))
                exit(0);
            else
                usage(argv[0]);

    return 0;
}

This is only the main source (probably enough). The complete code including the class definition can be found on Github.

Answer 1

I won't talk about the big picture (design), but here are some details where you can enhance your code:

• The "iterator" for loop was idiomatic a few years ago:

  for(std::list<Queue>::iterator it = listQueue.begin(); it != listQueue.end(); ++it)
  

  But now we have the awesome range-based for:

  for(auto&& elem: listQueue)
  

  It will do almost the same thing, except that all the iterator boilerplate is hidden. Also, it will compute listQueue.end() only once, so it could even be faster.

• tokens.at(0) can be replaced by tokens.front() which could be considered more expressive.

• Also, instead of repeatedly computing tokens.at(0), you could store it once in a variable and compare this variables to the different possible values. I am not sure whether the compiler can automagically do that for you.

• You are using std::string but you didn't include the header <string>. The header <string.h> only imports the C-style string handling functions.

• Generally speaking, when including the headers from the C standard library, try to include <cxxx> instead of <xxx.h> to be sure that every function is in the namespace std::.

• Since you implemented everything in one file, this is OK, but if you ever have to write header files, then, at all cost, avoid using namespace std; in header files to avoid potential name clashes due to global namespace pollution.

Answer 2

The lack of curly braces here has a potential for errors:

if (argc < 2)
    /*TODO daemon */
    cout << "Starting server\n";
else
    for(int i=1; i<argc; i++)
        if (!strcmp(argv[i], "-s"))
            shell();
        else if (!strcmp(argv[i], "-s"))
            exit(0);
        else
            usage(argv[0]);

The if statement may execute the output statement, but if you were to put some code on the same line as that comment, it would no longer execute the previous statement.

The else and the for should really have curly braces. This may confuse others and could still break if changes are made carelessly.

This is what it could look like (using your existing style):

if (argc < 2) {
    /*TODO daemon */
    cout << "Starting server\n";
} else {
    for (int i=1; i<argc; i++) {
        if (!strcmp(argv[i], "-s"))
            shell();
        else if (!strcmp(argv[i], "-s"))
            exit(0);
        else
            usage(argv[0]);
    }
}

Even the inner if/else statements should have curly braces, but you get the idea.

I've also noticed a lack of curly braces with your other for loops. Make sure this is applied everywhere to minimize error-prone code.

Related: curly braces and Apple's SSL bug

Answer 3

I still find your code very C like (not C++ like). If I was going to use a word to describe the difference is that C is very imperative while C++ is more declarative.

Example:

Your code to tokenize the stream is:

void tokenizeCmd(const string &str, vector<string> &tokens, const string &delimiters = " ") {
    string::size_type lastPos = str.find_first_not_of(delimiters, 0);
    string::size_type pos = str.find_first_of(delimiters, lastPos);

    while (string::npos != pos || string::npos != lastPos) {
        tokens.push_back(str.substr(lastPos, pos - lastPos));
        lastPos = str.find_first_not_of(delimiters, pos);
        pos = str.find_first_of(delimiters, lastPos);
    }
}

I would have done:

std::stringstream          tokenStream(str);
std::vector<std::string>   tokens(std::istream_iterator<string>(tokenStream),
                                  std::istream_iterator<string>());

Admittedly my plain standard version only supports space as a delimiter, but it is not that hard to add the extra functionality to use other delimiters.

You also have start learning the standard algorithms. If you find yourself looping over a container there is probably an algorithm that will do it for you.

        for(std::list<Queue>::iterator it = listQueue.begin(); it != listQueue.end(); ++it)
            if(it->GetName() == tokens.at(1))
                listQueue.erase(it++);

        // Can be expressed more tightly with:

        std::erase(std::remove_if(listQueue.begin(), listQueue.end(),
                                  [&tokens](Queue const& q){return q.GetName() == tokens.at(1}),
                   listQueue.end());

       // The difference here is compleity.
       // The original version is O(n^2) while using algorithms is O(n)

I would also like to point out a small bug:

            if(it->GetName() == tokens.at(1))
                listQueue.erase(it++);         // it++ here is a bug.

       // Note: the erase works correctly.
       //       also the increment works correctly.

       //       But now `it` points at what would have been the next element.
       //       This could even be `end()`
       //       But the `for(;;it++)` is applied to the end of the loop.
       //       If we are now at end() this is undefined behavior.
       //       If we are not at end() then we have just skipped an element in the
       //       list.

You are accessing elements via at(). You only do this if you need to check the range, otherwise you should be using operator[]() to access the elements.

    if(tokens.at(0) == "list") {

    } else if(tokens.at(0) == "add") {

    } else if(tokens.at(0) == "remove") {

    } else if(tokens.at(0) == "update") {

    } else if(tokens.at(0) == "help") {

    }

I would probably have tried:

    if (tokens.size() == 0) {/* Generate Error */break;}
    if(tokens[0] == "list") {

    } else if(tokens[0] == "add") {

    } else if(tokens[0] == "remove") {

    } else if(tokens[0] == "update") {

    } else if(tokens[0] == "help") {

    }

I would have personally used the command pattern:

    auto action = actions.find(tokens[0]);
    if (action == actions.end()) {
        action = actions["help"];   // default action if none found.
    }
    action->do(tokens);

Global variables!

static list<Queue> listQueue;

Really don't do that. This should have been your top level object around which you built some methods to manipulate it.

Answer 4

Don't forget about lambdas! Lambdas can greatly simplify the design of data structures: you create a map() function that takes a lambda, and then most of the operations that need to be done to the data structure can be done by invoking map() with different lambdas. The following code demonstrates some of my favorite C++11 features.

Note that I used the verbose name apply_to_all() instead of map():

/// This is a demonstration of a few nice C++11 features: auto, the ':'
/// iterator, initializer lists, std::function, and lambda expressions.  In
/// many cases, it's possible to achieve the same effect as I present using
/// even less code, but by being explicit I hope I have made the code and
/// comments easy to follow

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

using namespace std; // I'm being lazy here... should explicitly use cout,
                     // endl, function, map, pair, string, and vector


/// wrapped_map_t extends map<string, string>.  We start with a map of
/// key/value pairs, where both the key and value are strings, and we add a
/// new method to it.  The new method, apply_to_all, lets us apply a lambda
/// to (a copy of) every key/value pair that is in the map.
///
/// my claim is that lambdas fundamentally change the art of data structure
/// design.  To support my claim, I will show how this one simple function
/// obviates many public functions that one might want int a data structure
/// (count, print, extract_keys, find_matching_keys, etc).
struct wrapped_map_t : public map<string, string>
{
    /// the apply_to_all function simply iterates through the key/value pairs
    /// of the map, calling lambda(key, value) for each pair.  Strictly
    /// speaking, std::map is a red/black tree, and this will do a pre-order
    /// traversal
    void apply_to_all(function<void(const string, const string)>& lambda)
    {
        // note the use of C++11 'auto' keyword to avoid having to specify
        // the type of the iterator
        for (auto i = begin(), e = end(); i != e; ++i)
            lambda(i->first, i->second);
    }
};

int main()
{
    // declare an instance of the extended map
    wrapped_map_t kvpairs;

    // populate the map with some keys and values.  We're going to use some
    // great C++ magic here: initializer lists.  We don't even have to know
    // the type of the list, but the compiler will deduce that it's something
    // like pair<string,string>[]
    kvpairs.insert({{"donald", "duck"}, {"mickey", "mouse"},
                    {"minnie", "mouse"}, {"bat", "man"},
                    {"super", "man"}, {"milo", "otis"},
                    {"mighty", "mouse"}, {"rocky", "squirrel"},
                    {"bullwinkle", "moose"}});

    // 'func' is a reference to a function that takes two strings as its
    // parameters, and returns nothing.  This is a lot nicer than the old C
    // syntax in use before C++11 (i.e., void (*func)(string, string)).
    function<void(string, string)> func;

    // let's assign a new lambda to func.  the lambda will simply print its
    // parameters
    func = [](string k, string v) { cout << "{" << k << ", " << v << "} "; };
    // now let's apply func to every element in the set.  No need for a
    // "print_in_sorted_key_order" function in our class.  It would be
    // trivial to add "print keys" and "print values" features without
    // changing the data structure... we'd just need to pass different
    // lambdas
    cout << "KVPairs = ";
    kvpairs.apply_to_all(func);
    cout << endl;

    // The last example was pretty boring.  A function pointer could have
    // done that easily.  Now let's pass in a function that has a captured
    // reference to a variable that is local to this function.  In that way,
    // each time the lambda is called, it can access the function 'my_count'
    // that we declare *right here*.  The net result is that we can count the
    // elements in the map, without requiring map to provide a "count()"
    // function.  Note that the parameters to the lambda aren't used, and
    // that's OK.
    int my_count = 0;
    func = [&my_count](string k, string v) { my_count++; };
    kvpairs.apply_to_all(func);
    cout << "Elements in map = " << my_count << endl;

    // OK, so we could have achieved that effect with an apply_to_all that
    // took three parameters: string, string, int&.  But would we really want
    // to do that for every possible pass-by-reference type?  For example,
    // here we pass a vector of strings so that we can copy out all keys from
    // the map
    vector<string> keys;
    func = [&keys](string k, string v) {keys.push_back(k);};
    kvpairs.apply_to_all(func);
    // print the keys, to show that it worked
    cout << "Keys in map = {";
    for (auto i : keys)
        cout << i << ",";
    cout << "}" << endl;

    // note, too, that we can use the lambda to capture multiple local
    // variables, and that some can be pass by value while others are pass by
    // reference.  Here 'pre' is pass by value, 'keys' is pass by reference,
    // and our function copies into 'keys' all keys in the map whose values
    // begin with 'mo'
    string pre = "mo";
    keys.clear();
    func = [&keys, pre](string k, string v) { if (v.find(pre) == 0) keys.push_back(k);};
    kvpairs.apply_to_all(func);
    cout << "Keys whose values start with 'mo' = {";
    for (auto i : keys)
        cout << i << ",";
    cout << "}" << endl;

    // this example is much like the prior one.  Here we extract all
    // key/value pairs where the key begins with 'mi'
    pre = "mi";
    vector<pair<string, string>> pairs;
    func = [&pairs, pre] (string k, string v)
        { if (k.find(pre) == 0) pairs.push_back(make_pair(k, v)); };
    kvpairs.apply_to_all(func);
    cout << "Elements whose key begins with 'mi' = {";
    for (auto i : pairs)
        cout << "{"<<i.first<<", "<<i.second<<"} ";
    cout << "}" << endl;

    // the point of this example is to illustrate that when the lambda is
    // called with the key and value, the function we are executing *cannot*
    // modify the internals of the map.  Put another way, the lambda does not
    // become a member function of the class.  So, for example, if we wanted
    // to add a suffix to every key, then the only way we could do it would
    // be by explicitly updating the k/v pair in the map.
    string suf = " <3";
    // note: we need to capture *kvpairs itself* by reference
    func = [&kvpairs, suf] (string k, string v) { kvpairs[k] = v+suf; };
    kvpairs.apply_to_all(func);
    // use a lambda to print it out
    func = [](string k, string v) { cout << "{" << k << ", " << v << "} "; };
    cout << "Modified kvpairs = ";
    kvpairs.apply_to_all(func);
    cout << endl;
}

To compile, using a modern gnu toolchain (4.7 or later), add the "-std=c++11" flag.

Answer 5

I would like to add to all the other excellent answers:

Your usage of erase() in processCommand is not correct. Once erase has deleted an element the iterator is no longer valid. Instead you should use the iterator that is returned by erase():

  for(std::list<Queue>::iterator it = listQueue.begin(); it != listQueue.end(); )
  {
    if (it->GetName() == tokens.at(1))
    {
       it = listQueue.erase(it);
    }
    else
    {
       ++it;
    }
  }

Although as somebody else already noted, you should checkout the <algorithm> header. When you master algorithms and containers you are going in the right direction learning C++.

The using namespace std at the top of the file can be replaced with using statements inside the functions which makes the usage clearer. If you write using namespace std at the top you risk name collisions or worse in your code .

e.g.

void shell()
{
  using std::string;
  using std::cout;
  using std::cin;

  string strCmd;

  cout << "Switch to interactive shell\n";
  cout << "qsh> ";

  while (getline(cin, strCmd))
  {
    processCommand(strCmd);
    cout << "qsh> ";
  }
}

Also it is good when you create your own code to place in its own name space, that way you can be a bit more flexible with names and it is also more clear for users of your code where it comes from.

Your code is structured as a C program with normal functions like initialize(), shell() etc. Instead you should/could put it all into an own class which you create an instance of in your main() function.

e.g.

int main(int argc, char** argv)
{
  MyTokenizer myTokenizer(argc, argv);
  return myTokenizer.Run();
}