jeudi 24 août 2017

Is it a good or bad practice to create local functions using lambdas?

Take the following code:

void myOperation(Message& msg)
{
    if (msg.getInt("oldStatus") == 1 || msg.getInt("oldStatus") == 1001)
    {
        msg.setInt("total", 0);
        msg.setInt("avg", 0);
        msg.setInt("sdev", 0);
        logDebug("message values initialized.");
    }

    if (msg.getInt("oldStatus") == 1 || msg.getInt("oldStatus") == 2)
    {
        msg.setString("status", "pending");
        msg.setString("description", "has been set to pending by myOperation");
        logDebug("message set to pending.");
    }
}

The "initialize" and "set pending" blocks are repeated. Traditional solutions to this would be:

a) Create private methods - however I don't link that in this situation as they will only be used for the purpose of this one method. I think such methods "pollute" the mind-space of the reader of the code, especially if he was reading the class but not for the purpose of understanding this particular "myOperation" method.

b) Create a separate class for the utility methods - however, I don't think this is a good reason to create a new class (though yes, you can argue it will allow me to unit test them separately. Also see (a)

c) Re-arrange the code as below:

void myOperation(Message& msg)
{
    switch(msg.getInt("oldStatus"))
    {
    case 1:
        // initialize
        msg.setInt("total", 0);
        msg.setInt("avg", 0);
        msg.setInt("sdev", 0);
        logDebug("message values initialized.");

        // Set pending
        msg.setString("status", "pending");
        msg.setString("description", "has been set to pending by myOperation");
        logDebug("message set to pending.");

        break;

    case 2:
        // Set pending
        msg.setString("status", "pending");
        msg.setString("description", "has been set to pending by myOperation");
        logDebug("message set to pending.");
        break;

    case 1001:
        // initialize
        msg.setInt("total", 0);
        msg.setInt("avg", 0);
        msg.setInt("sdev", 0);
        logDebug("message values initialized.");
        break;
    }
}

But for me the last one above is a very unnatural way of writing the code. I think the switch, with the "oldStatus" cases is the most natural way.

So I would like to write it like this:

void myOperation(Message& msg)
{
    auto setPending = [](Message& msg) {
        msg.setString("status", "pending");
        msg.setString("description", "has been set to pending by myOperation");
        logDebug("message set to pending.");
    };

    auto initialize = [](Message& msg) {
        msg.setInt("total", 0);
        msg.setInt("avg", 0);
        msg.setInt("sdev", 0);
        logDebug("message values initialized.");
    };

    switch(msg.getInt("oldStatus"))
    {
    case 1:
        setPending(msg);
        initialize(msg);
        break;

    case 2:
        setPending(msg);
        break;

    case 1001:
        initialize(msg);
        break;
    }   
};

I've very rarely seen this approach being taken: the definition and usage of lambda's inside a function in this manner. My question is, are there any disadvantages of using this coding mechanism?

Note, my example is a rather contrived one, and there may be better ways of writing that particular code. It's there only as a potential use case for the coding mechanism I'm asking about.

Aucun commentaire:

Enregistrer un commentaire