1
public class Connection : IConnection
{
    public Connection(Uri baseAddress, ISerializer serializer)
    {
        BaseAddress = baseAddress;

        Serializer = serializer;

        ResponseFactory = new ResponseFactory(serializer);
    }

    private Uri BaseAddress { get; set; }

    public IResponseFactory ResponseFactory { get; set; }

    private ISerializer Serializer { get; }
}

In this code (redacted for brevity), the Connection class has two dependencies that it gets via constructor injection.

It has a third dependency, on IResponseFactory, where property injection is available but a default is created via the line ResponseFactory = new ResponseFactory(serializer) in the constructor. I suppose the view was that ResponseFactory is a suitable 'local default', to use Mark Seemann's terminology, but a user of the Connection class is free to provide an alternative IResponseFactory should they wish.

On revisiting this code after some time, I'm ambivalent about the the creation of the local default in the constructor and its dependency on ISerializer. While ISerializer is required in its on right by the Connection class, 'chaining' it together with ResponseFactory seems wrong. It feels like the Connection class should require an IResponseFactory via the constructor and forego the convenience of the local default and its dependency on ISerializer, that just happens to also be injected into this class.

What do you think? Is this a defined 'smell'?

  • 1
    It seems odd to me. I would prefer overloading the constructor (or optional parameter with default value), and make the response factory { get; private set; } set only in a constructor, unless we really know that we want this aspect of the object to be (dynamically) mutable. – Erik Eidt Apr 4 '18 at 15:00
1

Yes, it does look a bit odd for a number of reasons.

  1. Mix of property and constructor injection
  2. Is ISerializer a real requirement if its only used in ResponseFactory?
  3. Always create a ResponseFactory even if its never used?
  4. What happens if I switch the ResponseFactory around mid lifecycle of Connection?

Standard 'Code Smell' disclaimers

  • a 'code smell' only indicates a possible problem, it doesn't mean there is definitely a problem.
  • we cant see the whole code, maybe this way is needed for some reason
  • Thanks for the answer. ISerializer is a real requirement, it's used elsewhere but the code sample has been stripped down to aid focus. Likewise, ResponseFactory is used elsewhere but not shown. Regarding switching IResponseFactory, this is due to the simplistic implementation of that dependency's property injection. That's a different problem! Why do you consider the mixing of property and constructor injection to be a problem? – David Osborne Apr 4 '18 at 11:08
  • That addresses most of the code smells. Yes, Ideally I would have everything one way or the other. Preferably constructor as it enforces the dependency. You havent marked the property as 'required to be set' in any way, so I would not think to use it as an injection point – Ewan Apr 4 '18 at 11:14
0

The question is, whether the ResponseFactory is necessary to do anything meaningful or not.

YES? Then it should be provided via constructor injection. On the one side, it is nice, that the constructor provides a somehow sensible default. But on the other side, the constructor injection makes things explicit.

NO? Then there is no need for a locale default. A simply uninitialized field would be sufficient - at the cost of a null check. When it isn't needed, and in case it is, but it was overlooked there should be a clear signal (other than a NPE) that the caller hasn't done his homework.


Regarding @Ewan's answer:

Mix of property and constructor injection

I see no problem in providing necessary dependencies via constructor injection, while optional ones via setter injection.

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.