3

A water user can submit an Application for a water right with the hope of getting a Permit to use water, which might later become a BonaFideWaterRight. The right holder may apply to Transfer any of the above items (or others not listed for brevity) by changing ownership, moving it to new ground, splitting it in half and selling the remainder to another individual, etc...

The above-emboldened states of being for a water right (and other non-water-right things as well) have come to be known here as Processes. All of the above processes have lots of individual work items (sub-processes? But confusingly they're still referred to as Processes) in common, but the only one we need concern ourselves with here is the PointOfDiversion.

I'm in the midst of an effort to refactor code that I inherited regarding these processes.

First the abstract parent classes I've created (omitting a fair amount of ISomethingProcess interfaces being inherited along the way) . . .

public abstract class WREditProcess : IWREditProcess { }

public abstract class WaterRightsProcess : WREditProcess 
{ 
    public IWaterRightLookupRepository QueryWaterRights { get; }
    protected ILocationQueries LocationRepository { get; }

    protected WaterRightsProcess(IWaterRightLookupRepository queryWaterRights, ILocationQueries locationRepository)
    {
        QueryWaterRights = queryWaterRights;
        LocationRepository = locationRepository;
    }
    /* Work performed in virtual methods using those repositories */
}

public abstract class PointOfDiversionProcess : WaterRightsProcess, IPointOfDiversionProcess  
{
    protected IPODLocationRepository PODLocationRepository { get; }
    protected IPointOfDiversionRepository PODRepository { get; }

    protected PointOfDiversionProcess(IWaterRightLookupRepository queryWaterRights, IPODLocationRepository locationRepository, IPointOfDiversionRepository pointOfDiversionRepository)
        : base(queryWaterRights, (ILocationQueries)locationRepository)
    {
        PODLocationRepository = locationRepository;
        PODRepository = pointOfDiversionRepository;
    }
    /* Work performed in virtual methods using those repositories */
}

There's a large amount of concrete work done in those abstract classes using the repositories passed in from their child classes' constructors. This continues to the concrete classes (the one for transfers shown here in its entirety) . . .

public class TransferPointOfDiversionProcess : PointOfDiversionProcess
{
    protected override ILog Log => LogManager.GetLogger(typeof(TransferPointOfDiversionProcess));

    /// <summary>
    /// Constructor for a TransferPointOfDiversionProcess
    /// </summary>
    /// <param name="baseWaterRightRepository">Repository for base water right information</param>
    /// <param name="locationRepository">Repository that abstracts the locPODTransfer table (if such a thing existed, but instead it's a clump of XML)</param>
    /// <param name="pointOfDiversionRepository">Repository that abstracts the PointOfDiversion table</param>
    [SuppressMessage("ReSharper", "SuggestBaseTypeForParameter")]
    public TransferPointOfDiversionProcess(ITransferRepository baseWaterRightRepository,
        IPODLocationRepository locationRepository,
        TransferPointOfDiversionRepository pointOfDiversionRepository)
        : base(
            baseWaterRightRepository,
            locationRepository,
            pointOfDiversionRepository)
    {
    }

    /// <inheritdoc />
    public override string DisplayName => "Transfer";

    /// <inheritdoc />
    public override string ConfigLayerID => "locPODWRTransfer";

    /// <inheritdoc />
    public override string Name => "locPODWRTransfer";

    /// <inheritdoc />
    public override string CorrelateProcessName => "Transfer";
}

Note that the constructor for TransferPointOfDiversionProcess asks for a concrete TransferPointOfDiversionRepository class rather than the IPointOfDiversionRepository interface that its parent specifies. This is critical -- especially for transfers because the TransferPointOfDiversionRepository overrides all sorts of things from its parent because transfers are stored in a wholly different way from everything else. For the same reason, I'm planning a similar TransferPointOfDiversionLocationRepository class to take the place of the IPODLocationRepository parameter as well but haven't gotten there yet.

ReSharper tickles me with the "Parameter can be declared with base type" warning on this parameter, suggesting the IPointOfDiversionRepository type be used instead. I disabled this warning for each constructor, but now I can't shake the feeling that I'm getting this warning because of design flaws--failing to abstract something away or the need for some other pattern to indicate clearly the need for a specific implementation of an interface or something like that--but I can't figure out what. Can anyone suggest improvements (or, even better, tell me not to put so much faith in ReSharper)?

migrated from codereview.stackexchange.com Jan 3 at 1:43

This question came from our site for peer programmer code reviews.

3

What you're seeing is not a warning, but a hint. Check out this page and search for "SuggestBaseTypeForParameter."

This is the lowest severity level. Code issues with this severity simply bring your attention to a particular code detail and/or recommends a way of improvement.

It's not saying that you're doing something wrong. It's just possible that you might be able to use the base type (interface) for the parameter and didn't realize it. But in this case you do realize it and you have your reasons, so you could ignore it or disable it.

2

ReSharper is right about it and I've got the impression that you haven't quite understood how inheritance works yet...

Note that the constructor for TransferPointOfDiversionProcess asks for a concrete TransferPointOfDiversionRepository class rather than the IPointOfDiversionRepository interface that its parent specifies.

This is critical -- especially for transfers because the TransferPointOfDiversionRepository overrides all sorts of things from its parent because transfers are stored in a wholly different way from everything else.

This reasoning is wrong because using the base type is the whole purpose of inheritance. If you cannot use the base interface IPointOfDiversionRepository here then there is no point in having it in the first place.

TransferPointOfDiversionRepository overrides all sorts of things from its parent

Then this concrete implementation will use these overrides too even if you specify just the interface. This is exactly how it's expected to work. You might not be aware of that yet but you want to use this interface which provides you a certain level of abstraction and allows you to switch to a different implementation of it anytime without much effort. If you bind your constructor to a concrete type you're done and you can throw the interface away.

In your case you could just remove the TransferPointOfDiversionRepository parameter and hardcode it and it wouldn't change anything. You cannot provide a different implementation for it anyway so there is no point in having it at all.

However, if you want to be able to make everything testable and be able to use a mock implementation of the IPointOfDiversionRepository here, then you should definitely go with an interface and not a concrete type. It would only limit your options for testing and exchaning it for other implemenations.

1

The underlying problem is you try to use inheritance to apply a constraint to a property in base class. This is fundamentally not possible,

The property PODRepository is defined as IPointOfDiversionRepository in the base class, but you want the subclass to ensure that it is "actually" a TransferPointOfDiversionRepository, which is a more specific type. This have some major problems:

  1. A method in the base class may legitimately set the property to a different object which implement IPointOfDiversionRepository, which breaks your assumption. You cannot prevent that in a subclass.
  2. If you want to call some method specific to TransferPointOfDiversionRepository in the subclass, or pass it to a method accepting a TransferPointOfDiversionRepository, you have to use a cast.

Basically, if you want to ensure that a property is of a certain type, you have to define it as that type. This means the subclass should have its own field or property defined as TransferPointOfDiversionRepository. It is possible that TransferPointOfDiversionProcess shouldn't be a subclass of PointOfDiversionProcess at all.

So yes, the ReSharper warning does indicate a design problem, but it only flags a symptom rather than the root cause.

  • This answer and @t3chb0t led me to start refreshing myself on composition and also to restart my refactoring effort taking smaller bites – id est laborum Jan 5 at 3:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.