3

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.

  • 1
    Is that the only public member of AuditInserter? That looks like a rather roundabout way of writing dataContext.AuditTable.Insert(person, action); – Caleth Jun 13 '18 at 10:35
  • It lets you have private members share common parameters between calls without losing its "static" aspect, I suppose, rather than having to pass every parameter to every method which uses them. I'm not a fan though. – Neil Jun 13 '18 at 11:05
  • IMHO, I think that the original developer has his/her design a bit backwards. The instance method should invoke the static method, not the other way around as he/she has it. Thereby, eliminating the need for unnecessary object creation in the static method. Other than that, I would question the need for this abstraction given your arbitrary example, as no logic is actually being encapsulated. – Joshua Jones Jun 13 '18 at 12:10
  • The example I've used is deliberately simple. In the real code, the InsertAudit method would call other private methods. – amarsha4 Jun 13 '18 at 15:09
  • Unrelated: your example code won't compile because of the return in void Insert(AuditableAction action) – Thomas Schremser Jun 14 '18 at 9:37
6

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

  • 4
    I assume that the poster removed substantial bits of the code before posting it, so I'd be reluctant to call it deeply confused and confusing just from the posted code alone. However, if the code is exactly as written, then I must agree. – CharonX Jun 13 '18 at 14:01
  • Also, In addition to your example of two AuditableAction in a single AuditInserter, I find the possibility of a AuditInserter without a AuditableAction even more concerning - assuming the AuditInserter has more functions a minefield of temporal coupling could open up. – CharonX Jun 13 '18 at 14:06
  • 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. – amarsha4 Jun 13 '18 at 15:12
  • 2
    @amarsha4, Ah, you missed out rather an important part of the reason why your colleague is doing this then. Normally, I'd expect those private methods to be static and dataContext etc, to be passed to each method as parameters. By creating an instance an storing them as fields, those parameters can be "avoided". I dislike this approach, but others like it. One things's for sure, it has nothing to do with factories. – David Arno Jun 13 '18 at 20:52
  • @amarsha4, I have rewritten my answer in light of your comment. It would be useful if you recorded that important detail in your question too, please. – David Arno Jun 14 '18 at 9:17
4

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.

  • When I saw the title my first thought was factory pattern. I've done it myself on occasion when I want to enforce some sort of constraint in advance, or want to return different objects based on a parameter. – Loren Pechtel Jun 14 '18 at 2:13
1

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());
    }
}
0

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.

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.