CODEWITHSUNDEEP
c#interface
Ihor Bats
Ihor Bats

Reputation: 169

Why it's not ok to use interface in this case

I have the following situation:

interface IBaseReader{
   string Header {get; set;}
   string CurrentRow {get; set;}}

class BaseReader: IBaseReader{
   ......}

interface ICustomReader{
   string Header {get; set;}
   string Delimiter {get; set;}}

class CustomReader :BaseReader, ICustomReader{
   ......}

In the file proccessing class I am using this ICustomReader to know if derived instance from BaseReader is ICustomReader and if it's than I know that it contains delimiter and I can validate it. 

IBaseReader _reader;
if(_reader is ICustomReader)
    {
       var items = (_reader as ICustomReader).Header.Split((_reader as ICustomReader).Delimiter);
       //split the current row and header by delimiter and check if it is ok
    }

My friends says that its not ok to write such a code because we are checking if instance that is currently represented as IBaseReader is ICustomReaderthat appears in derived class and it's means that base class should know something about derived class. I general We have developed better solution for this. But I am just wondering what exactly it breaks and what will be the right solution in this case, if we just can't move property between classes?

Thanks!

Upvotes: 0

Views: 123

Answers (3)

Chris
Chris

Reputation: 27599

One major thing that this sort of code will have problems with is fragility.

IF you change ICustomReader you should only need to recompile that class and anything that has references to it. This is because only it and anything that uses it explicitly should really know anything about its inner workings (such as its properties).

If you imagine that your BaseReader is in one assembly and the ICustomReader is in another then any change to the ICustomReader (eg it no longer has a delimiter) then it will cause your BaseReader to break for no reason.

If instead you had a virtual method on BaseReader called Validate or similar then it could be called where you are doing the type specific validation currently. In the base class this method may do nothing if there is no validation to be done. Then in the concrete implementation of ICustomReader you can override the Validation to do whatever specific validation you want to do. This way the base class only knows about what it is and what its interface is and what its behaviour is and anything else is worried about by the specific implementations of child classes which know what they do and what needs validating on them.

This has the added benefit that if you add an ICustomReader2 then you will just need to write that class and not have to go and add extra logic into BaseReader. Imagine if you had 50 custom readers how messy that code would be with all manner of if statements checking for specific types. The solution to that messy code would be to refactor your long line of ifs into separate methods, one for each type. Then it is an easy thought of why not put these methods onto the child classes.

The other main reason to do this is to make things simpler to maintain. Somebody new to your code might wonder where the validation for an ICustomReader is? Well, its on that class of course. The base class isn't where you would expect it so it will confuse people if it is there.

Good code structure is a fairly large and weighty topic and I've tried to put across some informal arguments that might help you intutively see why that is a bad idea. If you want to google for more info a useful term would be "tightly coupled" and http://en.wikipedia.org/wiki/Coupling_(computer_programming)#Disadvantages lists a few of the other disadvantages.

Upvotes: 1

nmclean
nmclean

Reputation: 7724

There is nothing wrong with checking for an additional interface to see if a property is available. The problem is that you have two separate interfaces with a Header property that don't represent two separate properties. It would be better if IDerived inherited IBaseReader:

interface IDerived : IBaseReader {
   string Delimiter {get; set;}}

Or if it was a completely independent interface that is only concerned with the delimiter:

interface IHasDelimiter {
   string Delimiter {get; set;}}

Then there is no need to cast to the new interface to access the Header property because you already have it on the current type of _reader:

var items = _reader.Header.Split((_reader as ICustomReader).Delimiter);

Upvotes: 2

Tony
Tony

Reputation: 10327

You are essentially coupling the base class IBaseReader to a derived class ICustomReader when the main reason for creating a hierarchy is to make code reusable.

Now, whenever you want to derive a new interface from IBaseReader you are going to have to include ICustomReader even if you don't need it.

Upvotes: 1

Related Questions