A few weeks ago a new junior developer joined my team and I was the one assigned to teach him the basics of the team and get his code quality up to standard (I value knowledge sharing and I love teaching).
During code reviews I had noticed a couple of things I thought were not good practices, and when I discussed the issues with the new developer he showed me some .Net FCL code to support his way of doing things.
Although it is true that the FCL code can be a good place to look at code and see how things are done there are two important things to keep in mind:
- The framework cose had to minimize breaking changes, which limits it’s ability to evolve through time
- There’s a lot of code in the FCL, written by different developers in different times, so it’s quality can vary.
There’s one particular example I gave him, and that I would like to share with you: The Stream abstract class.
Streams in C# are incredible important, and include many of the I/O interactions such as StreamReader, StreamWriter, FileStream, but not only I/O, for example MemoryStream and NetworkStream.
One way to see what’s problematic about the Stream abstract class is too look at a class which inherits from it, but was written a long period of time after the design of it.
We’ll look at NetworkStream as our example. Every stream has to imply a method called Seek, which moves our position in the stream. One example of an implementation of such this is in FileStrean, where we can move to any part of the file without having to go over the entire file. In particular we can go back to the start of the file, or jump straight to it’s end.
It’s safe to assume that whomever decided a Stream should have a Seek method had the idea if files inside his head, and that it’s fitted the examples that existed when they wrote the class.
But the design is of this (abstract) class failed tge test of time. It doesn’t make sense to go back or jump to the end of data coming from a network. So what did the designers of NetworkStream did? Simply throw a NotImplementedException.
Whenever someone throws a NotImplementedException it has to be one of two cases:
- There is a problem with the interface
- You shouldn’t have inherited from that interface in the first place
The reasoning being that the interface doesn’t suit you if you can’t implement some of its members. It’s a broken contract.
If someone receives an instance of type Stream, it has no way of telling if the call to seek would work or not. In the general case – do you expect to wrap every call to a method invocation of an interface in a try-catch block?
In the case of Stream the problem is that not every Stream is a seekable stream, thus having such a method on the Stream class is either limiting its possibilities to be extended or that an implementor won’t be able to implement some of it’s methods – putting the burden of checking for problems in the calling code.
This problem is a particular instance of not following one of the fundamental principles of OOP which is the interface segregation principles.
There are three takeaways I hope you will take from this post:
- When you’re in your training period take the time to learn with an open mind. I know that I didn’t know I wasn’t very good at the start, and this is a blind spot many of us have. Being closed minded to being thought is a missed learning opportunity.
- No code, company or developer are sacred. Everyone makes mistakes (part of being human). Use your judgment to decide your opinion on it, don’t just accept it.
- Understand the limitations that a framework code has, and strive to learn practices and improve by other means of looking at code. Books are such as clean by uncle Bob can be eye opening.
That’s it for today, hope you had enjoyed this post. As always, comments are welcome, let me what your thinking and share your opinion 🙂