Problem with understanding SRP

Question

I have a hard time understanding when the Single Responsibility Principle is correctly used. Consider the following code:

// --------------------------------------------------------------------------------------------------
void CTCPClient::Try_Send_Data(const char *Outgoing_Message)
// --------------------------------------------------------------------------------------------------
{
    Reset_Return_Value();
    Copy_Data_Into_Buffer(Outgoing_Message);

    try
    {
        if((strcmp(Outgoing_Message, "") != 0))
        {
            Send_Data();
            Keep_Alive_Counter = 0; // reset as a button has been pressed
        }

        if(Return_Value <  0)
        {
            throw SEND_DATA_TCP_ERROR;
        }
    }

    catch(EError_ID)
    {
        Error_Message = strerror(errno);
        LOG_WARNING(__SOURCE__ << Error_Message);
    }
}

If I would read this method for the first time, I would, as the name sugest, expect a try and catch statement. But before that I´m calling two additional methods (Reset_Return_Value(), Copy_Data_Into_Buffer()). Is the SRP already violated here?

Further in the method I'm setting "Keep_Alive_Counter = 0;". In my opinion, at least at this point the SRP has been violated in addition to the fact, that if I were to be someone who didn't write the code, would simply not expect this line to be there. The problem I have here is: If I do this in an explicit method, it will be a one line method, something I encounter more often, as I try to consinder the SRP. These methods still look weird to me and I haven't found an answer to when they are considered good pratice. Always? In some cases? Never?

This is my own code so feel free to criticize, as long as it is constructive.

EDIT: Keep_Alive_Counter and Return_Value are class attributes, which are changed/used by other methods.

Answer 1

When considering whether or not a function or a method violates the SRP, you should look not at the implementation of the method but at what it's supposed/promises to do. Consider the following two functions:

void printTime() {
    print(getTime());
    invade(POLAND);
}

void printTimeAndInvadePoland() {
    print(getTime());
    invade(POLAND);
}

Only the second function violates the SRP. printTimeAndInvadePoland has two responsibilities - to print the time and to invade Poland. printTime does not violate the SRP, as it has only one responsibility - to print the time. It has a different problem - unwanted side-effects. You just want to print the time and suddenly you started a world war. printTimeAndInvadePoland does not have unwanted side-effects because it's supposed to invade Poland - that side-effect is part of it's responsibility.

Let's look at your Try_Send_Data. It has one responsibility - to send the data. You are worried about the calls to Reset_Return_Value and Copy_Data_Into_Buffer and about resetting Keep_Alive_Counter. Does any of them violate the SRP? No - because they don't change the fact that Try_Send_Data's only responsibility is to send the data. So the question is - do they make the method deviate from it's responsibility?

• Reset_Return_Value(); - it looks like it refers to the return value of some lower level data transmission function that you call from Send_Data, and Send_Data is setting. This is not a violation of the SRP, but it's still a bad practice: Send_Data should either return that value or throw an exception if it indicates an error. Even if you keep Send_Data as is, if it guarantees to always set Return_Value you can move the if(Return_Value < 0) check to only happen when Send_Data is called, and then you won't have to reset it.

• Copy_Data_Into_Buffer(Outgoing_Message); - I see no problem here. While I don't know if it copies the data to or from Outgoing_Message, this copy seems like a necessary task for sending the data, which supports Try_Send_Data one responsibility.

• Keep_Alive_Counter = 0; - updating the keep-alive information is part of the data sending process, so there are no weird side-effects here, but I don't think this line belongs here. If you update the keep-alive info every time you send data(makes sense), it should be done inside Send_Data. If, like the comment indicates, you only update it when a button is pressed(WTF?), it should be done in the button press event handler.

Of course, if Send_Data is not yours, and Try_Send_Data is a convenience wrapper around Send_Data's awkward API, this may be acceptable - though you might want to rename Try_Send_Data, because right now it's name indicates it's just a variation of Send_Data that does not throw, when it clearly does more. If you can change Send_Data, consider moving some of Try_Send_Data's functionality(like copying the data to the buffer, resetting the keep alive counter, and throwing in case of bad return value) to Send_Data.

Answer 2

The answer to your question is very simple.

Martin [who coind the word] defines a responsibility as a reason to change, and concludes that a class or module should have one, and only one, reason to change. (source)

That means if your function, regardless of their name or what they promise to do, has more than one reason to change, then it violates the SRP.

As an example, a function that searchs for a user record in the database and returns the result, doesn't violate the SRP, even though it may have a long and complicated logic inside.

On the other hand, a function that would look up for the user and would return the user record or create one if couldn't find any matching records, violates SRP because the second function has two reasons for change 1) when the look up logic changes 2) when the record creation logic changes.

Answer 3

Your code is quite weird, because you are checking variables not defined in that method. I don't see whole class, but I can only assume that they are somehow changed in other methods. So, even that you tried to make it nice, you managed to create something that looks like spaghetti code.

To answer your question: yes that is SRP, but done in a very bad way.

I can recommend you to read "Clean Code", and redo your class.

Answer 4

I think SRP is not correctly used in here. Reset_Return_Value() and Copy_Data_Into_Buffer() can be used separately. You can set a variable to hold your values and use those two methods separately. In that way, if the mechanism of Reset_Return_Value() or Copy_Data_Into_Buffer() changes, it won't affect your code. Something like the following:

Reset_Return_Value();
Copy_Data_Into_Buffer(Outgoing_Message);
Try_Send_Data(Outgoing_Message);
// -------------------------------------------------------------------------  -------------------------
void CTCPClient::Try_Send_Data(const char *Outgoing_Message)
// --------------------------------------------------------------------------------------------------
{
  try
   {
    if((strcmp(Outgoing_Message, "") != 0))
    {
        Send_Data();
        Keep_Alive_Counter = 0; // reset as a button has been pressed
    }

    if(Return_Value <  0)
    {
        throw SEND_DATA_TCP_ERROR;
    }
}
catch(EError_ID)
{
    Error_Message = strerror(errno);
    LOG_WARNING(__SOURCE__ << Error_Message);
 }
}
// --------------------------------------------------------------------------------------------------
void CTCPClient::Reset_Return_Value()
// --------------------------------------------------------------------------------------------------
{
           //...code to reset the return value
}
// --------------------------------------------------------------------------------------------------
void CTCPClient::Copy_Data_Into_Buffer(const char *Outgoing_Message){
// --------------------------------------------------------------------------------------------------
{
              //.........code to copy the data into buffer
}