current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I have the following overloaded functions. All the code is the same as except for their format (%s v.s %lu), so I couldn't use templates.

Should I implement generic programming like this?

Edited : Actually I'm executing redisCommand() in set()

bool set(const string& key, const string& value)
{
    // some codes for input check

    redisCommand("SET %s %s", key.c_str(), value.c_str());

    // some codes for making return value
}

bool set(const string& key, unsigned long value)
{
    // some codes for input check

    redisCommand("SET %s %lu", key.c_str(), value);

    // some codes for making return value
}

Both key and value might have space or double quotes.

set("my key", "my value");
set("my \"key2", 12345);
set("my key3", "my \"value2");

I would like to change set() as follows.

template <typename T>
bool set(const string& key, const T& value)
{
    // some codes for input check

    std::ostringstream cmd;
    cmd << "SET  \"" << escape_func(key) << "\" \""
        << escape_func(value) << "\"";

    redisCommand(cmd.c_str());

    // some codes for making return value
}

template <typename T>
const T escace_func(const T& v)
{
    return v;
}

template<>
const string escape_func(const string& str)
{
    replace all " to \" and return; 
}

Any suggestion would be appreciated.

share|improve this question
2  
You don't need generic programming for this. Using a std::stringstream will pretty much solve this problem for you. –  Yuushi yesterday
2  
This question, as it is currently written, is marginally on-topic for Code Review. It would have been better to post a more detailed code excerpt — perhaps an entire class definition — so that we can give you better advice. –  200_success♦ yesterday
1  
After editing, that's much better as a Code Review question! –  200_success♦ yesterday
add comment

2 Answers

up vote 5 down vote accepted

I'd avoid the problem completely by building the string using a stringstream, then passing a complete result to redisCommand. There are a number of ways to do this. One that works fairly nicely for this sort of situation is to create a manipulator to actually execute your command, then pass it to a stringstream:

#include <sstream>
#include <iostream>

using std::ostringstream;
using std::ostream;

// a manipulator to execute the string in a stringstream:
ostream &exec(ostream &s) {
    ostringstream &os = dynamic_cast<ostringstream>(s);

    redisCommand(os.str().c_str());
    return s;
}

// An operator to accept and invoke the manipulator:
ostream &operator<<(ostream &s, ostream &(*manip)(ostream &)) {
    return manip(s);
}

With those in place, we can write a small template that lets us pass either type for the second parameter, and handle things from there:

template <class T>    
bool set(string const &key, T value) {   
    // ...
    ostringstream cmd;
    cmd << "SET " << key << " " << value << exec;    
    // ...
}

Of course, if you want to also allow a variable number of parameters of varying types, you can turn this into a variadic template to allow that as well (but it's a little more complex, and not immediately obvious that it's necessary, so I'll refrain for now).

share|improve this answer
add comment
up vote 3 down vote

If possible, the command should be built as a list of strings. Concatenating your arguments into one long command string makes me suspicious, because it converts structured data into a degraded form. For example, if any key or value happens to contain a space, then it will appear to be two arguments.

Advice below applies to Rev 5 of the question:

You haven't provided enough detail in the question to tell what you're using the result for, but I'm going to guess that it will be a shell command to be executed. In that case, there may be all kinds of security issues as well, if a key or value contains characters with special significance to the shell, such as > (redirection to a file) or ; (delimiter between two shell commands). If a hostile user is able to specify key or value, then you have an arbitrary command execution vulnerability. You can avoid this problem by using the execv() family of functions, which take an array of arguments, instead of the system() function, which expects one concatenated string.

share|improve this answer
    
+1. You're right!. key or value can contain <kbd>space</kdb> or double quotes. I have updated my question. Even tough I have accepted Jerry's answer, your answer helped me a lot. Thank you. –  InoS Heo yesterday
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.