2

I have an abstract base class that will be used in hundreds of derived classes, including an additional abstract class.

There are at least 2 properties (let's call them Purpose and Description, both strings) that will be added to many (but not all) of the concrete derived classes, so I created interfaces (IPurposeful and IDescribable) to add them when needed. All is well so far.

I want a single method that I can call on all classes derived from my base class that will validate and update the Description property if it is indeed IDescribable, or just return true if it is not IDescribable. I'd like another similar method to validate/update the Purpose property.

I achieve this with a method in the base class that looks something like this:

protected bool CheckDescription(bool modify = false)
{
    if (this is IDescribable ele)
    {
        var newDesc = GetCorrectDescription();

        UpdateDescription(newDesc, ele.Description, modify);

        return newDesc.Equals(ele.Description);
    }
    else
    {
        return true;
    }
}

SonarQube marks the "this is IDescribable" check as a blocker (bad practice) and I am wondering why? The only other way I can think of to replicate this functionality would be to change the base method to this:

protected virtual bool CheckDescription(bool modify = false)
{
    return true;
}

and then add this exact same method to potentially hundreds of derived classes:

protected override bool CheckDescription(bool modify = false)
{
    var newDesc = GetCorrectDescription();

    UpdateDescription(newDesc, Description, modify);

    return newDesc.Equals(Description);
}

Now THAT would seem like bad practice.

EDIT: Changed the is/as pattern to remove redundancy

DanK
  • 59
  • 7
  • Use some thing like `if (GetType().IsAssignableFrom(typeof(IDescribable)))` – NaDeR Star Feb 15 '19 at 19:18
  • 1
    I don't have SonarQube but did you try to make a reform? the `is` and `as` are redundant here anyways. Try something like `var ele = this as IDescribable; if (ele != null)` – yazanpro Feb 15 '19 at 19:19
  • 1
    It is a code smell, and a somewhat illogical design. If an (derived) instance of the base class should have behavior that is specific of it being IDescribable, then the base class should implement IDescribable. If the base class should not be a IDescribable, why should it be concerend with being an IDescribable? If a derived class implements IDescribable, then the derived class should take care of the business of it being an IDescribable... –  Feb 15 '19 at 19:24
  • I would suggest to derive another Describable base class from your normal base class, which can implement the IDescribable interface as well as the protected method(s). Any describable concrete class then will derive from this Describable base class instead of the original underlying base class... –  Feb 15 '19 at 19:27
  • What kind of class needs "hundreds of derived classes"? Also you could make a common (abstract) base class for all the classes which have a description instead of adding the logic to the base class. – R1PFake Feb 15 '19 at 19:27
  • Hi yazanpro, when I changed to your suggestion Visual Studio is actually recommending I change it to if (this is IDescribable ele) { ... which eliminates your redundant concern (thanks!) but seems to just be the same result. – DanK Feb 15 '19 at 19:28
  • Hi elgonzo, the problem is that I have at least 2 and probably more properties that need very similar functionality and with your suggestion who knows how many permutations of base classes I would need instead of this simple solution that SonarQube does not like. – DanK Feb 15 '19 at 19:32
  • You're inverting the dependency relationship, and having the parent depend on its children when choosing how `CheckDescription` will behave. –  Feb 15 '19 at 19:34
  • 1
    If you just want to avoid the SonarQube message then you could remove the CheckDescription method from the base class and add it is a extension method instead, but then you would have to make it public or internal. – R1PFake Feb 15 '19 at 19:35
  • Hi NaDeR Star, I looked up the IsAssignableFrom method and it seems to me that if I pass that check I will still need to use as to convert to IDescribable and then check for null and then proceed as before. – DanK Feb 15 '19 at 19:36
  • Hi Amy, yes I have read that inverting the dependency is considered bad but I am trying to understand why it is bad in this case. What kind of problems can arise from this? – DanK Feb 15 '19 at 19:41
  • Simply adding an implemented interface to a subclass will alter the parent classes behavior. That is going to lead to unexpected bugs in the parent from seemingly irrelevant changes to children. Plus, what happens if the subclass doesn't inherit from the interface, but its subclass (a grandchild of the base) *does*? –  Feb 15 '19 at 19:42
  • Hi R1PFake, to answer your first question, there are hundreds of different element types in some software my company uses that all have a lot of common functionality yet each have specific functionality as well. There is also some functionality that is common to many but not all of them, like these Description and Purpose properties. All of these different element types need to be validated in their own ways and without getting polymorphic the solution would be worse than doing nothing with lots of switch statements each with hundreds of cases. – DanK Feb 15 '19 at 19:52
  • Also R1PFake, as for your suggestion about extension methods, I explored that idea but it doesn't seem anywhere near as elegant as the current solution. Maybe I just don't understand how extension methods would be implemented in this case... – DanK Feb 15 '19 at 19:52
  • Hi Amy, it would seem to me that the test (is this IDescribable) would work on a grandchild even if its parent was not IDescribable. Also, can you provide an example of the implemented interface altering a parent classes behavior? After all, that is exactly my desired outcome. – DanK Feb 15 '19 at 19:58
  • Why do you need an example? What you have in your question *is* that example. –  Feb 15 '19 at 20:22
  • Hi Amy, I am trying to understand _why_ it is a bad thing. It is exactly what I desire. I was wondering if you had an example showing _undesirable_ behavior because currently I see only positives. – DanK Feb 15 '19 at 20:27
  • 2
    I already said "That is going to lead to unexpected bugs in the parent from seemingly irrelevant changes to children". –  Feb 15 '19 at 20:34
  • 1
    You have two properties at the moment like this and indicate that there may be more in the future like this. It suggests to me that this base class is *incoherent*. It doesn't know *what it wants to be*. I'd strongly look at interface only, not using base classes. – Damien_The_Unbeliever Feb 15 '19 at 20:37
  • Hi Damien, if I went interface only, I would have to duplicate the code in hundreds of places. That seems to me to be much worse than the current solution. – DanK Feb 15 '19 at 20:52
  • Hi Amy, I still don't see a "bug." Class A derives from base and is not IDescribable so calling CheckDescription does nothing but return true. Class B derives from base and IDescribable so calling CheckDescription does what it is supposed to do (GetCorrectDescription and UpdateDescription are overriden in Class B if needed). Class C (grandchild of base) derives from Class A and IDescribable and functions similar to Class B. I don't see an example where this pattern breaks down. Thank you for your patience, I really would like to understand. – DanK Feb 15 '19 at 20:59
  • `Now THAT would seem like bad practice.` The challenge sometimes is that you make a bad decision early. Then you run with it for a while (creating hundreds of classes that inherit from it). Then you run into a wall. At that point, things that aren't great look good _in the context of the earlier bad decision_. That is partly why you are struggling to understand other people's points of view on this. You are seeing it mainly from one point of view (this change I am thinking about looks worse than what I have now). They are seeing it from the other (you made a bad early decision). – mjwills Feb 15 '19 at 21:16
  • Hi mjwills, I absolutely would love a better way. Perhaps if I describe my situation a little better it would be more useful for determining where I went wrong: I am working with software that has its own proprietary database. In this database are thousands of different element types (objects) each of which has a list of attributes (properties). A hundred or so of these object types should be set according to company standards and I am writing software to implement those rules. Many of them have a Description that needs to be checked against their own implementation of the Description rule. – DanK Feb 15 '19 at 21:53

2 Answers2

2

If your class may be IDescribable and may be IPurposeful, then give it sensible default implementations of those interfaces that may be no-ops and allow your descendant classes to override those implementations as required.

No type checks required. Just call the overridable methods and abide by their outcomes.

If your "optional" interfaces don't allow sensible no-op implementations, re-visit their definitions.

Now your base class can rely on the interfaces, rather than testing for them.

You may introduce runtime errors here - but the same can be said about base class methods that require a specific interface to be implemented that aren't compile-time checkable.

Damien_The_Unbeliever
  • 234,701
  • 27
  • 340
  • 448
  • The problem is that hundreds of derived classes _will_ be IDescribable and will be overridden in exactly the same way. Same for IPurposeful and possibly others in the future. This means I the code I have in one place (the base class) will have to be duplicated in hundreds of derived classes. – DanK Feb 15 '19 at 21:06
  • 1
    No, you provide one default implementation of `IDescribable` that does the no-op. If you have multiple specializations that *don't require the no-op* and share exactly the same implementation, you've improperly broken situations down into class requirements. – Damien_The_Unbeliever Feb 15 '19 at 21:10
  • 2
    @DanK - and I would ask you to *take a step back*. Your question is already "I'm sure I'm right" and your comments seem to be similarly oriented. Many of us are trying to tell you you've picked a bad design. If we don't all criticize **all** aspects of the design, you seem to be trying to pick parts off in a piecemeal fashion. – Damien_The_Unbeliever Feb 15 '19 at 21:14
  • I can't use a no-op implementation. I need these methods to check the current value, determine the proper value, and update the current value to the proper value if needed. Checking and updating will be exactly the same for all derived classes that happen to be IDescribable. Determining the proper value will be very different depending on the derived class. So no-op is not going to work. That leaves me with "re-visiting their definitions" but I don't see a better way. Everything else seems to lead to much worse code from what I have seen. – DanK Feb 15 '19 at 21:16
  • I am sorry if my posts come off that way. I assure you I do not think I am right. People much smarter than I have determined this to be a code smell, and a very bad one to boot. I am trying to understand the why and how I can make the code better. – DanK Feb 15 '19 at 21:18
  • No, an `IDescribable` no-op is either "my current value is always right" or "whatever you pass in is the right value". You do one or the other of these and document it as the default behaviour. – Damien_The_Unbeliever Feb 15 '19 at 21:22
  • Also, I just noticed the comment that "base class methods that require a specific interface to be implemented that aren't compile-time checkable." These methods do not require an interface to be implemented. They just act differently depending on whether the interface is implemented or not implemented. Also, thank you for even spending your valuable time on this problem. I really do appreciate it. – DanK Feb 15 '19 at 21:24
  • Ah, interesting. I will investigate this no-op pattern you describe. Thank you again! – DanK Feb 15 '19 at 21:26
  • Well, it looks like no-op will not work. I must validate the value when it is passed in... – DanK Feb 15 '19 at 21:59
0

I decided to explore extension methods a bit more and found that for my particular needs they will work fine. I wanted to mark the original suggestion to look at extension methods as the correct answer but cannot seem to find a mechanism to do so.

Anyway, extension methods fulfill my requirements to be able to define a default algorithm in one place for most derived classes and override that behavior in specific derived classes only if/when necessary.

The one downside is that I cannot call the "base" implementation from within the "override" method. This may lead to a need to duplicate code in the future but thankfully for now I have not had that problem.

DanK
  • 59
  • 7