SOLID : My view on SRP

SOLID is an acronym of acronyms. Each letter in the acronym is in itself already an acronym.

In this article I would like to share my view on the first letter of the acronym:

SRP : Single responsibility Principle

In my honest opinion there are very good explanations on the internet and in books, but nevertheless many people fail the understand this principle. In my opinion that is because there is something missing in the explanation.

Uncle Bob already pointed out that many people mistakenly interpret it that a class should be responsible for doing a single thing. That is already a problem.

So the best definition is as follows : A class should have only one reason for change.

That is already a lot better, but the statement is vague and subject to interpretation to many people.

In my opinion what is missing in the explanations is the notion of the abstraction level. A class should try to stay at the same abstraction level and should not be bothered with implementation details of a lower layer. If a class knows the nitty-gritty details of too many things. If a class knows the details of many things that don’t really belong there, then you are very likely violating SRP, and the chance that you have to add another abstraction layer is very likely.

Let us put this to an example: It is based on code that I really came across, and each time I see it, it makes my heart skip a beat. Now in the following code samples I will simplify things to make my point, I know that there are even better solutions to solve what I am about to show. Just assume that this is just the first iteration.

A.hpp

class A{
public:
    unsigned short member1;
    unsigned short member2;
};

B.hpp

class B{
public:
    unsigned char* serialize(unsigned char* buffer,const A& a) const;
};

B.cpp

unsigned char* B::serialize(unsigned char* buffer,const A& a) const{
  unsigned short counter = 0;
  buffer[counter++] = (unsigned char)((a.member1>>8) & 0xff);
  buffer[counter++] = (unsigned char)(a.member1 & 0xff);
  buffer[counter++] = (unsigned char)((a.member2>>8) & 0xff);
  buffer[counter++] = (unsigned char)(a.member2 & 0xff);
  return buffer[counter];
}

 

clearly the intent of class B is to serialize the content of class A into a memory buffer and it is considering little endian and big endian, the buffer is filled in network order. The buffer that is returned is pointing to the next free location so other classes can be serialized in similar way. This is something that is done very frequently in code when handling protocols. It can even go as far as to even consider booleans and the trick is of course that there is a counter part piece of code that is able to deserialize the code to make a new instance of class A, perhaps on another computer, or perhaps the buffer was written to disk, and is loaded again. If the logic for reading and writing is similar, this is certainly possible. The code can be loaded on a different computer that is either big endian or little endian, it would work.

Now this code here, I hope I did not make any typo’s but it should work. But look at how many details and possibility for errors there are. Even if class B only has the serialize method in my opinion is it is already violating SRP, and OCP too. SRP is violated because the responsibility for B is to serialize and the only reason for B to change is in my opinion if a member is added or removed to class A.
The problem with this kind of code is the following list of things that the developer has to keep in mind when coding this kind of mess:

  • counter must be incremented all the time, forget this one time and the code breaks.
  • The braces are very important because the operator precedence takes the & before the >> byte shift operator. Believe me, this is something which is easily overlooked.
  • The serialization code knows all the details of class A. It knows the size and type of both members. Why on earth should class B know all these details about class A. It should know very little about class A.
  • As a result of the above the code in the implementation file is easy to break, if we change member1 in class A to an int or an unsigned long, then we have to change the implementation as well.
  • There is already repetition of code in B, because both member1 and member2 are being handled the same way. So there is copy paste code even in the same method.
  • I have found this code for int64 as well, then you need 8 lines of code and the byte shifting must be correct with multiples of 8 this way. Again this is error prone.
  • I have found that many developers are a bit lazy, and have a strong tendency to copy paste this kind of code. Hell it is only 2 simple members anyway, we are talking about 4 bytes, are we really going to add another abstraction layer for something simple as that ? Now if you start copy pasting and considering that the code is error prone, if there is an error in the original code where you copied from, then the error is in 2 locations. If this error is detected in the first location, all the places where the code was copied are not going to be modified. The designer in question is just not aware of how many times it was copied. Unless he is good designer and starts looking, but a good designer would never create code like this in the first place.
  • Now instead of talking about it, let’s just show my point by changing class A a little:

A.hpp

class A{
public:
    unsigned int member1;
    unsigned short member2;
};

B.cpp needs to be changed to handle the change of type of member1!

unsigned char* B::serialize(unsigned char* buffer,const A& a) const{
  unsigned short counter = 0;
  buffer[counter++] = (unsigned char)((a.member1>>24) & 0xff);
  buffer[counter++] = (unsigned char)((a.member1>>16) & 0xff);
  buffer[counter++] = (unsigned char)((a.member1>>8) & 0xff);
  buffer[counter++] = (unsigned char)(a.member1 & 0xff);
  buffer[counter++] = (unsigned char)((a.member2>>8) & 0xff);
  buffer[counter++] = (unsigned char)(a.member2 & 0xff);
  return buffer[counter];
}

This is really problematic, I had to add 2 lines of code, and if deserialization is also needed also there 2 additional lines of code are needed.

A better way to achieve this is using a memory stream. Many would argue that a class should be able to serialize itself into a buffer. In this particular case A does not have real responsibilities and this would be correct. But I am assuming that A does have other responsibilities and is not just a stupid POCO class. In that case adding serialization and deserialization to A would break SRP. Although I don’t want to be an evangelist. There are more harmful ways of violating SRP.

I am not going to show the code for the memory stream but it is clear that the memory stream is the class which is responsible for actually filling the buffer. And the nice thing about it is that it can be reused. Moreover, streaming is a topic with many possibilities, I am merely touching the surface here.

So now it is time for cleaner code and to show the advantages:

B.cpp

unsigned char* B::serialize(unsigned char* buffer,const A& a) const{
  memstream s(buffer,sizeof(A)); 
  s << a.member1;
  s << b.member2;
  return buffer[s.getPos()];
}

Of course the ugly code with the byte shifting and the like is now moved to the implementation of the memory stream, but that is a good thing. Because this is just one location. If there is a bug, it must be fixed in just one location.
If you ask me the code using the memory stream is more readable anyway. And the nice thing is that there is room for improvement if we allow the interface of the methods to change a little:

B.cpp

stream& B::serialize(stream& s,const A& a) const{
  s << a.member1;
  s << b.member2;
  return s;
}

This latter class is even more defensive. The thing is that the memstream has methods to check if it is still valid. It is a wrapper around a buffer, and you have to pass the size of the buffer in the constructor. Now you can start using the << operator as much as you want, but if the buffer is completely filled, the isValid() member would return false, and the << operator does nothing as soon as there is not enough space in  the buffer. Of course at the end of serialization you have to check the validity of the stream. Also note that memstream is an implementation class. There is an abstract class stream from which memstream is derived called stream which is see appearing above. All of a sudden I can serialize B to different streams. So serialize now only knows about a stream and is using an abstract interface. (DIP) I can serialize to memory, to disk, to whatever I like.

Unfortunately I cannot post the code for the memory stream because it is owned by my employer and I do not have permission at this point to publish it.

Needless to say that the implementation of the serialize method using the stream class is at a higher abstraction level than the earlier examples, and indeed, the responsibility of the byte shifting and endianness problems is now moved to the stream class.

Conclusion : This is a good example of pointing out that staying at the right abstraction level is good addition to the explanation of SRP. In the examples shown you can even get a grasp that the SOLID design principles help out on each other as well. Because the OCP principle is improved in the examples as well. Only if a member to the class A is added or removed, only then do I need to change the implementation file. If the size of existing members change, no changes are needed, just recompilation.

 

 

 

Published by

stuyckp

I have been programming since my 15. Started on commedore 64 in basic. Learned myself OOP and I am passionate about programming and creating a good design according to SOLID design principles, modern techniques, and this of course involves continuous learning. My favorate programming languages are C# and C++. I don't very much like scripting languages. Code should be maintainable and readible at all times. When creating regular expressions in code that nobody understands, you are bound to forget how it works after a few months yourself.

Leave a Reply

Your email address will not be published. Required fields are marked *

This site uses Akismet to reduce spam. Learn how your comment data is processed.