Public static method calls private constructor

Question

I'm working in a C# codebase that was largely written by a former developer and this pattern is used extensively...

public class AuditInserter
{
    public static void Insert(
        DataContext dataContext,
        Person person,
        AuditableAction action)
    {
        return
            new AuditInserter(
                dataContext,
                person)
            .InsertAudit(action);
    }

    private AuditInserter(
        DataContext dataContext,
        Person person)
    {
        _dataContext = dataContext;
        _person = person;
    }

    private readonly _dataContext;
    private readonly _person;

    private void InsertAudit(AuditableAction action)
    {
        _dataContext.AuditTable.Insert(_person, action);
    }
}

// Consuming object
public class MainProgram
{
    public static void Main()
    {
        AuditInserter.Insert(
            new DataContext,
            new Person,
            new AuditableAction);
    }
}

The constructor is private and construction of the object is handled by the static method. Note that this is not an example of the singleton pattern - the object does not hold a reference to a static instance of itself.

Is there any benefit of this design? It would be my natural inclination to design it like so:

public class AuditInserter
{
    public AuditInserter(
        DataContext dataContext,
        Person person)
    {
        _dataContext = dataContext;
        _person = person;
    }

    private readonly _dataContext;
    private readonly _person;

    public void Insert(
        AuditableAction action)
    {
        return InsertAudit(action);
    }

    private void InsertAudit(AuditableAction action)
    {
        _dataContext.AuditTable.Insert(_person, action);
    }
}

// Consuming object
public class MainProgram
{
    public static void Main()
    {
        new AuditInserter(
            new DataContext,
            new Person)
        .Insert(
            new AuditableAction);
    }
}

I'm loathe to just blindly follow this existing pattern without understanding what benefits it offers (if any).

The code is in C# but this issue is not C#-specific.

Answer 1

(Answer rewritten in light of the comment, "Sorry, I should've mentioned that I made a very simple example to help show the main design element that confused me. The InsertAudit method would call several other private instance methods.". This clarification changes the static approach from "deeply confused and confusing" to being a viable solution to avoiding passing around parameters.)

From the example you give, your colleague's code is deeply confused and confusing. However, with your clarification that the InsertAudit method would call several other private instance methods, is becomes much clearer what's happening. All of the static code you show can be reduced to the following with no change of function:

public class AuditInserter
{
    public static void Insert(DataContext dataContext,
                              Person person,
                              AuditableAction action)
    {
        dataContext.AuditTable.Insert(person, action);
    }
}

But let's revisit that code and add some private methods to make it more representative of the real code. I've taken the liberty of using syntax features in C# 7 to reduce the size of the code to for brevity):

public class AuditInserter
{
    public static void Insert(DataContext dataContext,
                              Person person,
                              AuditableAction action)
        => new AuditInserter(dataContext, person).InsertAudit(action);

    private AuditInserter(DataContext dataContext, Person person)
        => (_dataContext, _person) = (dataContext, person);

    private readonly _dataContext;
    private readonly _person;

    private void InsertAudit(AuditableAction action)
    {
        DoSomethingFirst();
        DoTheInsert(action);
        DoSomethingAfter();
    }

    private void DoSomethingFirst()
        => // do something with _person and _dataContext here

    private void DoTheInsert(AuditableAction action)
        => _dataContext.AuditTable.Insert(_person, action);

    private void DoSomethingAfter()
        => // do something with _person and _dataContext here
}

Again that code can be simplified in one respect by just making it all static: we can get rid of the constructor and the fields. But we then increase complexity in another respect: the methods now have multiple parameters to allow those values to be passed around:

public class AuditInserter
{
    public static void Insert(DataContext dataContext,
                              Person person,
                              AuditableAction action)
    {
        DoSomethingFirst(dataContext, person);
        DoTheInsert(dataContext, person, action);
        DoSomethingAfter(dataContext, person);
    }

    private void DoSomethingFirst(DataContext dataContext,
                                  Person person)
        => // do something with person and dataContext here

    private void DoTheInsert(DataContext dataContext,
                             Person person,
                             AuditableAction action)
        => dataContext.AuditTable.Insert(person, action);

    private void DoSomethingAfter(DataContext dataContext,
                                  Person person)
        => // do something with person and dataContext here
}

Which approach you take is a mixture of personal preference and how many parameters you have in the real example. For just those two items, I'd stick with the fully static approach. If it increased to five, six or more, I'd likely adopt something similar to that former developer. Other folk will have other personal preferences and might even just adopt that caching of parameters approach for all cases (as your former developer seems to have done). Your version of the code behaves quite differently to either static version though. This can be shown by changing the Main to:

public class MainProgram
{
    public static void Main()
    {
        var inserter = new AuditInserter(new DataContext, new Person);
        inserter.Insert(new AuditableAction);
        inserter.Insert(new AuditableAction);
    }
}

We are now using the same AuditInserter to insert two actions. This may be a good thing, in which case you could switch to using your approach if you find it easier to work with. It may be bad though, in which case leave things as they are (or switch to a fully static approach).

One thing we can be sure on though (subject to you revealing some other important detail you mistakenly removed when simplifying the code example) is that this pattern is wholly unrelated to factories.

Answer 2

This may be an example of the factory pattern.

You can see small, but significant differences between your implementation and the current implementation: In your implementation it is entirely possible to create an AuditInserter without it containing one (and only one) AuditableAction.

• Nothing would stop you from omitting that Insert(new AuditableAction);
• Also nothing would stop you from calling Insert() on the same AuditInserter repeatedly.

Of course, you may archive the same by simply requiring the AuditableAction as part of the constructor for the AuditInserter, but the are sometimes reasons you do not wish to do that (e.g. the Insert() function may throw an exception, but you want to avoid the constructor of the AuditInserter to throw one, or you need to do more things with the freshly constructed object which do not belong in the constructor and/or must be executed in a certain order).

Similarly, there may be several flavors/subtypes of AuditInserter and the factory function can deliver you the proper type depending on your inputs (e.g. a FinancialAuditInserter if your AuditableAction is releated to finances, or a HrAuditInserter if it is related to HR, etc...).

In the end, it all depends on how it is used - every design pattern can be abused, and only in the context of the use in your application you can determine if it is sensible or not.

Answer 3

The design is a little bit confusing. The objective was to hide the implementation and the object creation step. But the implementation confuses the developers as the objective is not clear. I recommend using Extension Methods which is a simpler known approach. There is no need to introduce an extra layer of complexity which is the class "AuditInserter".

The method can look like

public static class DataContextExtension
{
    public static void InsertAudit(this DataContext context, Person person, AuditableAction action)
    {
        context.AuditTable.Insert(person, action);
    }
}

And the usage can look like

public class MainProgram
{
    public static void Main()
    {
        new DataContext().InsertAudit(
        new Person(),
        new AuditableAction());
    }
}

Answer 4

I'd agree that the use of a static method in that way is confusing and IMO should be re-factored or at least discouraged from progressing into new code. I will use factory static methods on data container classes where I want to ensure that the class is constructed in a valid state, never with something like that where there are dependencies. While you can wire up IoC containers to use delegates to call static methods, this somewhat defeats some of the benefit of using a container. (extra coding)

For example if I have a "result" type DTO that would have two states representing a successful call, or a failed one (with validation failure details) I will commonly use a private constructor with a Success static factory method to initialize an instance with the results, or a Failure static method to initialize an instance with the failure reasons. Simple data containers can serve as their own factory, or you can use a more pure factory pattern but then the classes can't be "locked down" quite so easily to ensure they always represent a valid state for their purpose.