4

I'm getting ready to release a feature to production tomorrow & feel I have misdesigned said feature. I'll try to explain my scenario clearly. I'm looking for alternative design suggestions to this. My real concern is my title - the "final" class takes two parameters of the same interface type, but they MUST be in the correct order...

I'm creating a mapping from string types to an enum meaningful within our system. So given a certain string, we should return the associated enum. Now, when we perform this lookup, our system will provide TWO strings. One of the strings is more specific than the other. So we should first look for a mapping for string A (more specific). If none is found, try to find a mapping for string B. For reasons that are difficult to get into, we NEED to have two separate mapping objects as the string mapping is a bit complex. One of my goals was to drive home this point & separate the definitions of the more specific mappings & less specific mappings. Here is my ultimate implementation scenario (rough C# pseudo-code, hopefully makes sense to other language users):

Interface IMap
{
    public enum GetAssociatedValue(string lookup);
}
Class MapBase : IMap
{
    protected override Dictionary<string, enum> MapAssociator;
    public enum GetAssociatedValue(string lookup)
    {
        // lookup logic
    }
}
Class MoreSpecificMap : MapBase 
{
    // This class only exists to define the mapping property, no behavior

    constructor()
    {
        // Here, we override the "MapAssociator" property from the base class & define all
       // values necessary
    }
}
Class LessSpecificMap : MapBase
{
    // This class only exists to define the mapping property, no behavior

    constructor()
    {
        // Here, we override the "MapAssociator" property from the base class & define all
       // values necessary
    }
}
Class LookupService
{
    constructor(IMap moreSpecificMap, IMap lessSpecificMap)
    {
        // Look, we need both the moreSpecific & lessSpecific maps, both of type IMap. But a 
        // user could pass these in the incorrect order & generate incorrect results, seems 
        // dirty
    }

    // Here as well, the caller of this function could pass the params incorrectly
    public enum GetMapValue(string moreSpecificString, string lessSpecificString)
    {
        var result = moreSpecificMap.GetAssociatedValue(moreSpecificString);
        if (result == null)
        {
            result = lessSpecificMap.GetAssociatedValue(lessSpecificString);
        }
        return result;
    }
}

Right now this "LookUpService" class is defined via DI where I manually new up instances of the MoreSpecificMap/LessSpecificMap classes so it is not being created all over the code, but it still seems like a maintenance problem & point of confusion for future maintainers. Any thoughts or feedback on this?

Also, ignore the interface & class names - they have more meaningful names within my systems domain.

7
  • 2
    I fail to see how two parameters of the same type, which are well named, lead to a maintanance problem. You have this all over the place in every existing piece of software. (Think createRange(from, to) or Math.pow(base, eponent), which is absolutely clear to every reader.)
    – mtj
    Feb 16 at 6:01
  • Yes, I may be worrying for no reason. It was mostly when I went to register it in DI. I couldn't just register my two instances of IMap & then trust the LookupService to be constructed correctly - I now had to manually specify the creation of LookupService - but this my not be problematic & is reassuring to hear that you may not think it is. I thought about doing a strategy pattern so the MoreSpecificMap & LessSpecificMap also had to define a type explicitly setting them apart, but decided against it. Wasn't sure if I would regret it or not Feb 16 at 6:08
  • 2
    I remember an interface where a 2d point was specified with two arguments h (horizontal) and v (vertical) and some genius changed it to (x, y). Still compiled. Results were completely wrong.
    – gnasher729
    Feb 16 at 6:45
  • @mtj: the difference is the two IMap objects behave differently. Two different integers do not behave differently. Feb 16 at 16:55
  • Do you need to mock the IMap objects for testing purposes? Do you have differing "specific" implementations, and differing "less specific" implementations? Feb 16 at 16:59

3 Answers 3

5

You register LessSpecificMap with your DI, you register MoreSpecificMap with your DI and then you write LookupService so that it has a constructor taking (MoreSpecificMap moreSpecificMap, LessSpecificMap lessSpecificMap). When you register that with DI, it works.

The only problem in your code is that you artificially created an interface that obfuscates your code. Delete IMap, it does not serve any purpose except causing you problems.

9
  • 1
    Using the concrete type for DI registration is not a great solution here, as it shuts the door on mocking and quick substitution of dependencies when needed. Similarly, the existence of IMap can be justified by the ability to mock IMap objects (which you can't do for MapBase without concretely inheriting MapBase itself).
    – Flater
    Feb 16 at 10:26
  • 3
    Well, if needed then there is no problem to just insert an interface for each class. I don't see the need to mock the class though. It seems super straight-forward. In testing, you can just use the original. I cannot see any reason to mock it. So for me, not having interfaces until I need them is the better design. When needed, you are definetely right though.
    – nvoigt
    Feb 16 at 10:29
  • 3
    Well, that is something I would just generally disagree with. Mocking is a tool that sometime is neccessary. Someone who mocks everything in the end has a huge bunch of tests verifying that their mocking framework has no bugs and very little percentage of code actually tests whether their own code runs. If an alternative implementation (whether it's mocking or just actually a domain decision) is neccessary, then the seperate interface for each class is neccessary. If not, then not. Doing something everywhere, "just because" is just code bloat.
    – nvoigt
    Feb 16 at 10:38
  • 1
    You generally don't need to write mock-testing code. Handling your mocked dependency entails direct control over its behavior/return values on a per-test basis, which is trivial enough to not warrant testing. There are plenty of mocking libraries that handle the implementation hassle-free as well. The effort of mocking is truly negligible; but the cost of having to introduce testing when your codebase never prepared for it is significantly non-negligible. I'm all about YAGNI but a minimal effort with significant future dividends is difficult to YAGNI away.
    – Flater
    Feb 16 at 10:56
  • 1
    I never suggested not testing. Or not switching implementations if needed for testing. I'm just saying that the words if needed are essential, because if mocking isn't needed, then it's waste/bloat. It doesn't matter how great or simple the framework is if it's not needed and code can be properly tested without it.
    – nvoigt
    Feb 16 at 10:59
2

I couldn't just register my two instances of IMap & then trust the LookupService to be constructed correctly - I now had to manually specify the creation of LookupService - but this my not be problematic & is reassuring to hear that you may not think it is.

As is the case with many things, automation often hinges on convention and assumption, and more niche scenarios are usually not accounted for.

In this case, DI containers tend to work under the assumption that the base type alone is enough information to know which concrete object to instantiate. In your case, this is not sufficient information, and therefore you cannot blindly rely on it.

That being said, some DI containers allow for additional information to help make the decision. For example, NInject's contextual bindings would allow you to differentiate between your primary and secondary sources. Snippet example from the link:

public class ReportController : Controller
{
    public ReportController([Named("replication")]IUOW replication)
    {
    }
}

public class NormalController : Controller
{
    public NormalController([Named("uow")]IUOW uow)
    {
    }
}

// DI Configuration

kernel.Bind<IUOW>().ToConstructor(i => new UOW(new DatabaseContext())).Named("uow");
kernel.Bind<IUOW>().ToConstructor(i => new SomethingElse(new DatabaseContext())).Named("replication");

A second solution would be to use marker interfaces:

public interface IPrimaryMap : IMap {}
public interface ISecondaryMap : IMap {}

These interfaces are empty, but inherit the mapper interfaces, and then your concrete classes inherit the marker interface:

// This remains unchanged
public class MapBase : IMap { /*...*/ }

public class MoreSpecificMap : MapBase, IPrimaryMap { /*...*/ }
public class LessSpecificMap : MapBase, ISecondaryMap { /*...*/ }

You register the dependencies using the marker interfaces:

myDIContainer.Register<IPrimaryMap,MoreSpecificMap>();
myDIContainer.Register<ISecondaryMap,LessSpecificMap>();

And then you use the marker interfaces as the injected type:

constructor(IPrimaryMap moreSpecificMap, ISecondaryMap lessSpecificMap)

The downside here is that marker interfaces are generally looked down upon. However, in my professional opinion there are cases where marker interfaces are a net positive when they easily solve an otherwise much more complex problem. I do try to avoid them as much as possible and not pepper them through a codebase (to avoid the straw that will break the camel's back), but when used sparingly and with reasonably justification, I don't oppose it.

On the upside, marker interfaces would be a valid solution regardless of which DI container you're using, as this ensures you're actually still only using the base type to declare the specific

Right now this "LookUpService" class is defined via DI where I manually new up instances of the MoreSpecificMap/LessSpecificMap classes so it is not being created all over the code, but it still seems like a maintenance problem & point of confusion for future maintainers.

As to the point of confusion, having clearly named constructor parameters should be sufficient here to avoid confusion. Maybe a bit obviously, I would also suggest making sure that the primary source comes before the secondary source, as this is what most people (if not everyone) would innately expect. Thirdly, I would suggest using primary/secondary or main/backup, as opposed to more specific/less specific. Just a matter of clear naming, IMO.

As to the maintenance problem; this is one of those cases where it's only a problem when there is no better solution. If your DI container allows for a better solution (e.g. the contexual bindings in NInject I linked to), then that is a better solution.
If, however, your DI container only allows for type-based registration, then having to manually specify the construction of your service is the best you can do, at which point it ceases to be a "problem". I would judge the marker interface solution about as well as the manual construction; so it's your call which you'd prefer to implement.

1
  • I think the YAGNI argument @nvoigt made is correct, when you don't need IMap. When you do, this works. Feb 16 at 22:15
1

Things to consider.

  1. Is there an "obvious" order? Like x, y or start, finish? If so, use that. But, be warned, not everybody agrees on what is "obvious". In early C many copy functions were (to, from). Latitude and Longitude are reversed in some systems. To stick with x,y I suppose.
  2. If the language supports named arguments, use them.
  3. Is one argument optional? Put optional arguments last, especially in languages that support defaults or varargs.
  4. Are these pairs of arguments used in other methods, like in a library you use? If so, use that order. For example, node.js callbacks always have err first. Golang convention puts err last.
  5. When all else fails, consider alphabetical order. Arbitrary but objective.
  6. Give them good names. If possible, do argument testing, e.g. instance of, at least in early code to catch dumb errors.
1
  • 1
    I like the use of named arguments to attempt to clearly communicate what the parameters mean Feb 16 at 17:28

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.