3

Which is the preferred design to use, one constructor that allows null, or two constructors where one throws an ArgumentNullException on null?

Two constructors with exception throwing

public class Foo
{
    private IDictionary<string, string> _bar;

    public Foo()
    {
        _bar = new Dictionary<string, string>();
    }

    public Foo(IDictionary<string, string> bar)
    {
        if (bar == null) { throw new ArgumentNullException(); }
        _bar = bar;
    }
}

One constructor that handles null

public class Foo
{
    private IDictionary<string, string> _bar;

    public Foo(IDictionary<string, string> bar = null)
    {
        if (bar == null) {
            _bar = new Dictionary<string, string>();
        } else {
            _bar = bar;
        }
    }
}
2
  • Are you sure you want to take over a mutable externally owned dictionary? Apr 7, 2016 at 13:13
  • 1
    You can have one constructor call the other, e.g. :this(new Dictionary<string, string>()) in the default constructor. Apr 7, 2016 at 13:25

5 Answers 5

4

I would recommend to use a factory method. You can state your intention (createEmpty(), createWithValues()) and expose what you expect from the caller: createWithValues() - null is not allowed. The constructor would then be private and only the createWithValues() would contain the null check or any other value check.

class Foo {
    private Map<String, String> bar;
    private Foo(final Map<String, String> bar) {
       this.bar = bar;
    }

    public static Foo createEmpty() {
       return new Foo(new HashMap<>());
    } 

    public static Foo createWithValue(final Map<String, String> values) {
       if (values == null) throw new YourFavoriteExceptionForThisCase();
       return new Foo(values)
    }
}

Simple and clean. (Sorry, code is in java)

8
  • 3
    A factory method seems like an overkill for such a simple question. I believe it would just add unnecessary complexity.
    – Fred
    Apr 7, 2016 at 12:48
  • But its not wrong, and, there are several resources which would encourage factory methods - even in this case. Downvote seems a bit strong, also because its a matter of taste. But I already read that this SO is very negative and harsh.
    – morpheus05
    Apr 7, 2016 at 12:53
  • Thanks for the answer. It's always interesting with different views. FYI, I did not downvote. I am new to this SO, so I don't know if its harsh.
    – Fred
    Apr 7, 2016 at 13:47
  • 2
    This code is pretty neat. I don't see how this is unnecessary complex. Some people simply just don't know when to use design patterns...
    – ironmouse
    Apr 7, 2016 at 16:16
  • 2
    Factory methods that express explicit intension and private constructors just like this example make code so much clearer, and IMO are therefore not an overkill. Remember code is read far more times than it is written so the clearer the intension the better because you are either writing for someone else to read or maybe even "future you" to read in weeks, months or years - when you may have forgotten the context. All to often we see code that is ambiguous.
    – Dib
    Aug 24, 2020 at 10:50
3

The simplest interface should be provided that covers all cases that result in valid state upon construction of your object. You should also be liberal in what you accept. Providing one constructor is always simpler than providing two, and writers of code that use this class won't be burdened with an extra decision.

If _bar is exposed in any way from the other methods/properties of your class, document your constructor with documentation comments on what the behavior is when null is passed in as the argument to your single constructor (that being that bar is empty by default if null is passed in).

1
  • 2
    You should also be liberal in what you accept +1 just for this. I am staggered how many seasoned devs would rather throw an exception for an unexpected null rather than try and continue gracefully. Tony Hoare called nulls his billion dollar mistake. That might be an underestimate in my view.
    – Robbie Dee
    Apr 8, 2016 at 11:23
0

Although use of null to indicate a default value is a well established principal reaching back to the C / C++ roots of C#, another approach could be to borrow the concepts of the Option type (called Maybe in Haskell)

This will force the caller to explicitly specify whether he wants wants the default or not. This avoids the confusion about deliberately passing null as a bad parameter and using null to indicate optional:

You would then have a single constructor which covers BOTH the concerns of your "two constructors" code.

public Foo(Option<IDictionary<string, string>> optionalBar)
{
    if (bar == null) 
       throw ArgumentNullException();
    if (bar.HasValue) {
        _bar = new Dictionary<string, string>();
    } else {
        _bar = bar.Value;
    }
}

Unfortunately, C# doesn't quite flow as nicely as say a FP language, which would use pattern matching to reason over Some<Dictionary> and None.

3
  • 1
    Most Option types provide .ValueOr(default)-like method. Use that.
    – Euphoric
    Apr 7, 2016 at 12:22
  • Good idea. I was thinking OP may be more used to the "HasValue" / "Value" semantics of Nullable<T>
    – StuartLC
    Apr 7, 2016 at 12:23
  • 1
    Also, you still need to provide the parameter-less constructor. IMO having to call Option<IDictionary<string, string>>.None on every other invocation is not nice. And default parameter is not supported in this case.
    – Euphoric
    Apr 7, 2016 at 12:28
0

I'm for the first case.

In the second case, it promotes null as a valid parameter. IMO null should not be used to indicate absence of value. That's why Option types exist (as hinted by StuartLC).

2
  • 1
    This is in C# though, which afaik do not have any Option type.
    – Fred
    Apr 7, 2016 at 12:46
  • @Fred There are tons of libraries that provide one.
    – Euphoric
    Apr 7, 2016 at 12:57
0

I'm not hugely blown away by either to be honest.

The first one can throw an unexpected exception and the second initialises a member variable which hasn't even been supplied as a parameter. Constructors should not yield unexpected behaviour.

I'd prefer something like this:

public class Foo
{
    private IDictionary<string, string> _bar;

    private IDictionary<string, string> Bar
    {
        get
        {
            if (_bar == null)
                _bar = new Dictionary<string, string>();

            return _bar;
        }

        set { _bar = value; }

    }

    public Foo()
    {

    }

    public Foo(IDictionary<string, string> bar)
    {
        Bar = bar;
    }
}

The user is free to either supply a parameter or call the parameterless constructor. And if by some oversight, the property is called without being initialised it creates one on the fly via lazy instantiation.

9
  • I'd avoid lazy initialization, unless it provides a significant performance advantage. One problem with your implementation is that it makes the class thread unsafe for multiple readers. Another issue is that you can assign null the the property but when you read it back, it's suddenly another value. Apr 7, 2016 at 13:11
  • Read the OP again - the intention is to defend against null. Why would I possibly want null back when it was passed in error in the first place?
    – Robbie Dee
    Apr 7, 2016 at 13:19
  • 1
    You should add an exception to the setter, so a caller can't set it to null. Otherwise you're violating the basic expectation of properties that the getter should return the value you called the setter with (or at least a value equivalent to it). IMO your code is much worse either of the OP's variants. Apr 7, 2016 at 13:23
  • Like the constructor case the property would only ever be set to null in error so no exception is required (or even desirable) here. Lazy instantiation isn't just about performance. It is about letting the program execution flow as much as possible whenever it is safe to do so. See LINQ's default methods for similar functionality.
    – Robbie Dee
    Apr 7, 2016 at 13:29
  • If by "in error" you mean "by a bug" then an exception is very much needed, so you can quickly detect the bug, instead of masking it. Apr 7, 2016 at 13:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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