Java – Public Static Method Calls Private Constructor

cconstructorsjavastatic methods

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.

Best Answer

(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.

Related Topic