C# – Using Instance or Static Method for reusable method

cperformance

I have made a lot of forms for my desktop application, some forms were using the same method which I have to copy and paste the code (not OOP).

Let's say I have a method called FirstDayOfWeek() and this method is needed in 5 forms, that means I need to copy and paste the code 4 times to the other forms.

What I want to ask is:

If I want to put this FirstDayOfWeek() in a new class, should I make it as default or static (Instance or Static)? So if there's an update for this method I don't need to tweak it in 5 forms.
Which has better performance; Instance, Static or just Procedural code (copy and paste)?

Best Answer

The answer to your question is simple, but without additional context it can lead to overapplication of the same pattern in more complex scenarios where it is no longer correct to do so.

The short answer

Statics are the way to go in this case. The main reason being that this is a helper method whose result is not bound by a given context.

The long answer

The correct method depends on the goal of the method and how it achieves this.

Static methods should be used in cases where the method's purpose is not contextual and globally applicable. The logic required to find the first day of the week is always the same.

However, suppose your users are able to choose which day is the first day of the week (Sat/Sun/Mon). Now your logic to find the first day of the week is contextual and no longer globally correct (it depends on the user). While this can still be done using statics by passing the user (or their chosen day); this becomes more cumbersome when you consider more complicated example.
The better option here is to use dependency injection to inject behavior. Create multiple classes, each being an implementation of the same base class. This is a very complex topic that I can't explain from scratch, but I hope the key difference (contextuality) is clear.

Performance

There is no meaningful difference between static and instanced methods. The cost of instantiating an object is massively negligible. The only thing that can impact performance is if you have expensive logic in your constructor, but that would imply that you need this expensive logic in the constructor and thus cannot work without it, so it's still not really an issue that you should work to avoid (any more than you otherwise would).

As an aside:

Procedural code (copy and paste)

That's not procedural code. Procedural code suggests that it is generated by some tool (pun not intended), not copy/pasted by a developer.

Copy/pasted code is never the right approach, unless you can prove that these multiple instances are functionally independent of each other. In other words: they can become very different things, and the fact that they currently do the same thing is coincidence; there is no requirement that when you update one you must update all the others.
Spoiler alert: 9 times out of 10 when developers think multiple instances of the same method is the right approach, it's not.

Related Solutions

Is It Bad Practice to Update an Object in a Static Method?

Note: this question may be better served in codereview.stackexchange.com

In general, I try to keep static methods side-effect free.

In your particular example, I would change it as follows:

public PackageStatus CheckShippingVendor(string shipmentId, mvFile shipmentFile)
{
    var packageStatus = new PackageStatus();
    using (var shipmentItem = shipmentFile.Read(shipmentId))
    {
        if(!ValidateDelivery(shipmentItem))
            shipmentItem.Message = "NO INFORMATION AT THIS TIME";

        shipmentFile.Write(shipmentId, shipmentItem);
    }
    return packageStatus;
}

private static bool IsDeliveryValid(mvItem shipmentItem)
{
    int shipped;
    var shippedResult = Int32.TryParse(shipmentItem[1].ToString(), out shipped);
    if (!shippedResult)
        return false;

    int delivered;
    return Int32.TryParse(shipmentItem[57].ToString(), out delivered);
}

This does two things:

It removes the side-effect from your static method
It reduces your validation method to a single responsibility: giving you a yes/no answer to whether or not a delivery is valid.

That said, I would iterate on the CheckShippingVendor method some more:

The name gives no indication that there are side effects.
It always returns an empty PackageStatus, rendering the return value useless.
It appears to have several responsibilities which could be broken into separate helper methods.

C# – Forcing Derived Class to Implement Static Method

Such methods shouldn't be static (what if you have to deal with two databases?) and they also shouldn't be on your entity classes.

Instead, you should have a separate type, that represents the storage for this entity. That type could be generic, so that code that's not entity-specific doesn't have to be repeated.

This way, inheritance works (storage for ClassA can inherit from some base storage) and you don't have to perform the nonsensical "I have to create a person to get a list of all persons".

This pattern is for example used by Entity Framework, there the storage type is called DbSet<T>.

Best Answer

Related Solutions

Is It Bad Practice to Update an Object in a Static Method?

C# – Forcing Derived Class to Implement Static Method

Related Topic