3
\$\begingroup\$

I'm having an argument with my boss about whether or not it's better to have a separate function for a single line of code.

The code reads something like:

while (count <= poscount)
{
    key = etc.substr(pos2prev+1, pos-pos2prev-1);
    std::string::iterator end_pos = std::remove(key.begin(), key.end(), ' ');
    key.erase(end_pos, key.end());

    val = etc.substr(pos+1, pos2-pos-1);

    //process key and val
    DEBUG("{\n\t%s:%s\n}\n", key.c_str(), val.c_str());
    json_object_object_add(paramList, key.c_str(), json_object_new_string(val.c_str())); 

    count++;
    pos2prev = pos2;
    pos = etc.find("=", pos+1);
    pos2 = (count == poscount) ? etc.find("\0",pos2+1) : etc.find("&", pos2+1);
}

Is it better coding style to include the line

json_object_object_add(paramList, key.c_str(), json_object_new_string(val.c_str()));

in a separate function name such as processKeyVal(), or leave it as-is?

\$\endgroup\$
1
\$\begingroup\$

I'm guessing that your boss's argument is that a well named function would make the code more readable, and I agree with that in general. I often do that myself even when the function contains a single line.

In this particular example I would not bother though. The comment above the json-call says the same thing as the function name would, and there are no obvious advantages to make a function for a single call.

So if your question is specifically for this example, then I agree with you - keep it as it is, but if you want a more general answer, then I agree with your boss.

For example - if this one-liner is something that you use in more than one place, i.e. if calling the "processKeyVal()" function in more than one place makes sense. Then I say you should make a function of it even if it would contain only one line of code.

Or to put it another way - it is not (always) the number of code lines that should determine if you make a separate function of a code snippet.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

Not a direct answer to your question and possibly far fetched but here you go:

Your code looks like it's parsing a string (presumably a query string) and serializes it into JSON. I would consider extracting the json serialization dependency out of it and pass in an abstract serializer. The line in question would turn into:

m_serializer.Add(key, val);

Some advantages of this method:

  • Separates the parsing of the code from the serialization
  • Allows you to easily add different forms of serialization without having to change the parsing code
  • Makes unit testing (in case you do have unit tests) cleaner because it would just test that the right methods with the right arguments are called on a mock serializer.

As it stands now I probably would not bother adding an additional method however I would introduce local variables to hold the arguments for the call instead. Can make debugging a tad easier.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ ...and readability++ :) \$\endgroup\$ – Mathieu Guindon Nov 17 '13 at 2:52
0
\$\begingroup\$

I would leave it as it is.

It's only 1 line, and it's a function call of itself and not an evaluation or an operation, so why wrap a function call within a separate function.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

Give it its own function. Since this is C++, you can inline it so that it won't incur any performance penalty or use a dangerous macro. If you decide you want XML or a binary format or anything else tomorrow, you will be glad you didn't commit the rest of your code to JSON by using json_object_object_add.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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