Breaking of LSP principle in the source-code

Posts   
 
    
kievBug
User
Posts: 105
Joined: 09-Jan-2009
# Posted on: 28-Jun-2012 22:52:54   

Hey guys,

I'm trying to provide my own implementation of provider classes: FieldInfoProvider, PersisitanceInfoProvider, and InheritanceInfoProvider. I can not use *Base version for obvious reason - it is limited to be initialized at the very beginning.

So I did my implementation of those but there are some issues. The code is full of places like these: DataAccessAdapterBase.cs:


IFieldPersistenceInfo[] persistenceInfo = new FieldPersistenceInfo[fields.Count];

or FieldInfoProviderBase.cs


private ElementFields ProcessEntityFields(IInheritanceInfoProvider inheritanceInfoProvider, Hashtable namesProcessed, string elementName)
{
...         
List<string> entityNamesOnHierarchyPath = ((InheritanceInfoProviderBase)inheritanceInfoProvider).GetEntityNamesOnHierarchyPath(elementName);

So it seems like you are using an interface but tying it to specific object - this is a code smell and breaking the of design principles and best practices.

Anton

Otis avatar
Otis
LLBLGen Pro Team
Posts: 39588
Joined: 17-Aug-2003
# Posted on: 29-Jun-2012 11:44:14   

First of all, of course, casting to a class when you pass in an interface is 'not ideal': why bother with the interface? True. I'll try to explain this below.

You have to know: if someone tells me 'this is a code smell' I'm not interested in reading any further as it's subjective though at the same time it's so negative, one can't discuss it any further, as it classifies the code as a hack in the person declaring the code smell, but perhaps there are no other options? simple_smile (I'll discuss this below).

'Best practices', I think you mean 'solid' ? yeah, well... what can I say... there are serious problems with using it practically in some situations. Take the example you quote. An interface is passed in, but further on it's cast to a base class. I.o.w.: you can't implement the interface again.

Fully agreed that this is not ideal. So, instead of pointing at it and calling it a code smell, let's look at the alternatives. I can be short: there aren't any which satisfy my requirements (which means: 'all public interfaces' isn't an option). The main issue is that the code itself has to access data / call methods which are internal. To do so, it has to either cast to a base class or cast to an internal interface. BOTH aren't available in the passed in interface: a public interface can't inherit from an internal interface, and of course casting to a class when you have an interface means you already assume the implementing type is that particular class.

This then only leads to the question: why the interface? Well, when the code was written we wrote interfaces for all the classes we use. As it turned out, we could also use the public class instead, because there's no other class implementing this interface. But, would that have made a difference?

No. The public class is in the generated code, so we can't use that in runtime code. The only solution here then would have been to use the base class InheritanceInfoProviderBase instead of the interface. Which would make no difference for you.

Is this all bad? No. The main reason is that these particular base classes are essential in the working of the framework. As a lot of their methods/properties are internal for obvious reasons (and using an internal interface and cast to that results in the same problem as you have now) so using the base class is logical in this case. The alternative would have been to use an internal interface. Same problem for you. As the two alternatives we had were: either cast to a base class or use an internal interface and cast to that (which is the same, the public interface can't implement/inherit the internal interface), we chose the former as it avoids the internal interface.

Would it have been nicer if we would have exposed the method with the base type instead of the interface? Sure. I'm not arguing against that. We didn't do that though as we now in theory have the choice to implement another class with the same interface and refactor the code which now casts to the base type without breaking a method's signature.

Now, of course, the 'design principles' might also suggest that all interfaces should be public as that would have solved it, but I firmly disagree. I don't want internals public, simply because it serves no purpose for us.

As you called my code to break with 'design principles' and 'best practices' and contains 'code smells', I'd like to hear from you how you would have solved this. simple_smile

Frans Bouma | Lead developer LLBLGen Pro
kievBug
User
Posts: 105
Joined: 09-Jan-2009
# Posted on: 29-Jun-2012 15:41:22   

Well, may be the post was too rude or so, if so, sorry about it.

As for me, when I design a framework or library, usually I plan for extensibility points. I'm trying to give developer some freedom(to some extent) to implement the contracts that I make public and do not limit to some specific classes that ships with a library. Sure it is very subjective and possibly personal to the implementer. Llblgen is really good and powerfull framework and I'm really happy that I've choosen it. It has a lot of extensibility points and this is one of the reasons for choose it amongst the others. May be it is the way I think but usually when I see an interface I think of it as a contract, and I assume that if I pass my own implementation to the method it will still work with it. And as for me that is the main point why we have interfaces, really. Is there any other reason? As for no, it is a contract. And that is exactly why I wrote my post. As for me it is up to the developer/architect/etc of the framework/library to make the design, API, interfaces and all the public/visible things so that all others knowing the same language, rules and principles may use it without any problems. Sure this is an ideal world :-).

So regarding the cases that I've mentioned and proposed solution:

  1. DataAccessAdapterBase.cs:

IFieldPersistenceInfo[] persistenceInfo = new FieldPersistenceInfo[fields.Count];

There are a lot of places where such initialization of the array for persistence infos is, and usually the array is filled by elements that is returned by IPersistenceInfoProvider which supposed to return an interface. I assume that the interface is a contract which I have to implement and I've implemented it and it doesn't work just because of that specific initialization of the array. As for me it is a very simple change - change initialization and that is it. I do not see any reason why it cannot be replaced with this:


IFieldPersistenceInfo[] persistenceInfo = new IFieldPersistenceInfo[fields.Count];

It compiles and seems to work correctly(I haven't tested everything though). May be I'm missing something, I'm definitely not a pro in the source code of llblgen yet :-).

  1. FieldInfoProviderBase.cs

private ElementFields ProcessEntityFields(IInheritanceInfoProvider inheritanceInfoProvider, Hashtable namesProcessed, string elementName)
{
...         
List<string> entityNamesOnHierarchyPath = ((InheritanceInfoProviderBase)inheritanceInfoProvider).GetEntityNamesOnHierarchyPath(elementName);

Well, here I'd suggest just removing the cast - the method is a part of the interface :-) so the cast is just useless. Most likely somebody just forgot to remove the cast when extracted the interface.

So both fixes are pretty simple to do, and it is really a code smell but not a design or any other smell, which is good :-).

Anton

Otis avatar
Otis
LLBLGen Pro Team
Posts: 39588
Joined: 17-Aug-2003
# Posted on: 02-Jul-2012 12:08:11   

kievBug wrote:

Well, may be the post was too rude or so, if so, sorry about it. As for me, when I design a framework or library, usually I plan for extensibility points. I'm trying to give developer some freedom(to some extent) to implement the contracts that I make public and do not limit to some specific classes that ships with a library.

Of course, but keep in mind: this framework is 9 years old simple_smile So it carries code which is newer than that of course, but also code which is as old as the first day the first class was written. Another point is that extension points are nice, but every extension point is a maintenance problem in the future: you can never ever refactor that maintenance point again, as doing so will break code. For an OSS framework just uploaded to github to give it a home somewhere, that's not a big deal. For a commercial framework which has thousands and thousands of projects build upon it, it's a different ballgame. We learned over the years that extension points are often going to bite you in the *** down the road, even if you think about them long and hard.

Sure it is very subjective and possibly personal to the implementer. Llblgen is really good and powerfull framework and I'm really happy that I've choosen it. It has a lot of extensibility points and this is one of the reasons for choose it amongst the others.

thanks simple_smile

May be it is the way I think but usually when I see an interface I think of it as a contract, and I assume that if I pass my own implementation to the method it will still work with it. And as for me that is the main point why we have interfaces, really. Is there any other reason? As for no, it is a contract.

Yes there are other reasons simple_smile Look at the collection classes. They're generic classes. The way generics work in .NET, it has to know at compile time the type of the generic type argument. This isn't always known. To overcome this, you can use interfaces to do a different kind of generic programming, which means you can work with generic classes without knowing the generic type argument.

Another reason is (but a bit moot, I admit) to use an interface and not the base class (which implements the interface) to have a public notion of the class interface which allows you to refactor inner code without breaking anything. This can be a bit overkill, if the base class is all you need and won't go away.

So regarding the cases that I've mentioned and proposed solution:

  1. DataAccessAdapterBase.cs:

IFieldPersistenceInfo[] persistenceInfo = new FieldPersistenceInfo[fields.Count];

There are a lot of places where such initialization of the array for persistence infos is, and usually the array is filled by elements that is returned by IPersistenceInfoProvider which supposed to return an interface. I assume that the interface is a contract which I have to implement and I've implemented it and it doesn't work just because of that specific initialization of the array. As for me it is a very simple change - change initialization and that is it. I do not see any reason why it cannot be replaced with this:


IFieldPersistenceInfo[] persistenceInfo = new IFieldPersistenceInfo[fields.Count];

It compiles and seems to work correctly(I haven't tested everything though). May be I'm missing something, I'm definitely not a pro in the source code of llblgen yet :-).

Good point. I'm not sure it will work along the way though. As I said, the core code is at places 9 years old, design choices from back then still leak through at places. We started with all interfaces, but ran into problems I described above: you have to cast to something internal if you want to use the internal interfaces, which is a thing that's unavoidable. So changing the above line will make things more in line with what 'should have been', internal code in FieldPersistenceInfo is still required along the way somewhere.

Newer code we added in recent years doesn't sport interfaces but just works with base classes, unless an interface makes it easier to work with generic types. It's however a problem in older code that we can't make go away: interfaces are used everywhere, but internally we have to cast to internal interfaces/base classes, which makes it moot to refactor it. But I simply don't know a solution to that, even if I refactor everything.

  1. FieldInfoProviderBase.cs

private ElementFields ProcessEntityFields(IInheritanceInfoProvider inheritanceInfoProvider, Hashtable namesProcessed, string elementName)
{
...         
List<string> entityNamesOnHierarchyPath = ((InheritanceInfoProviderBase)inheritanceInfoProvider).GetEntityNamesOnHierarchyPath(elementName);

Well, here I'd suggest just removing the cast - the method is a part of the interface :-) so the cast is just useless. Most likely somebody just forgot to remove the cast when extracted the interface.

Yes we refactored a lot in the last revision so I guess some code is still left there without reason. I'll correct it.

Still I'd like to know, how you'd solve this: say in some framework class, I need internal code of InheritanceInfoProviderBase, but I have the interface, not the base class. How to do that? IMHO the only solution is to make the internal code part of the public interface, precisely something I'd like to avoid (though then I need casts)

Frans Bouma | Lead developer LLBLGen Pro