Is there ever a valid reason to have a static method inside a non-static class?

Question

The title is pretty much my entire question.

I'm doing a code review for an off-shore developer who has several static methods inside a non-static class. Before I challenge the developer and mark this as "needs changing", I just want to make sure.

I understand the purpose of a static class: It can not be instantiated and can be used directly. But I can't see any reason to have a static method in a non-static class. Is there any valid use-case for this?

The methods in question are all private and are called from non-static methods.

Here's an example:

    public ViewResult ClaimDetails(ClaimDetails claim)
    {
        if (claim.ClaimNumber != 0)
        {
            claim = Get_ClaimDetails(claim);
        }
        return View("ClaimDetails", claim);
    }

    private static ClaimDetails Get_ClaimDetails(ClaimDetails claim)
    {
        ClaimsRepository claimsRepository = new ClaimsRepository();
        _claimDetails = new ClaimDetails();
        _claimDetails = claimsRepository.GetClaimDetails(claim.ClaimNumber);
        return _claimDetails;
    }

Comments are not for extended discussion; this conversation has been [moved to chat](https://chat.stackoverflow.com/rooms/207805/discussion-on-question-by-casey-crookston-is-there-ever-a-valid-reason-to-have-a). — Samuel Liew, Feb 14 '20 at 01:40

score 1 · Answer 1 · answered Feb 13 '20 at 14:15

1

For example, it's an often-used approach to have multiple static factory methods in cases where it is undesirable to expose the internal implementation.

    Car hatchback = Car.CreateHatchback();
    Car sedan = Car.CreateSedan();

In this example, both instances have a hidden internal implementation of factory methods, possibly instantiating specialized internal subclasses or calling internal constructors.

answered Feb 13 '20 at 14:15

Rumkex

21
2

How is this different from `public Car(CarType carType)`? This also hides any internal implementation behind the interface of the constructor. I could call 100 internal constructors and noone bound to the public interface would notice. – Markus Deibel Feb 13 '20 at 14:42
@MarkusDeibel: You're assuming that `CarType` is the only difference. If there is more than just a single value, there is much more value to hiding the precise implementation, hence why the factory pattern exists. Granted, this answer is using a poor man's factory by avoid the actual factory class and instead using static methods on the class itself; but the functional purpose remains the same. Public constructors are not always the best approach, especially when there is complex or highly repetitive initialization logic. – Flater Feb 13 '20 at 16:31

nvoigt · Accepted Answer · 2020-02-13T16:28:34.000

There are a few things that would raise an eyebrow with me, but the method signature is fine.

static is a keyword that makes sure the method does not use instance members. That's great. It clearly signals this to other developers. I would have argued for the opposite side: a method should be marked static, except when needed otherwise.

Many tools and even Visual Studio itself would flag it if you did not mark it static: CA1822: Mark members as static.

Quoting Microsoft:

Members that do not access instance data or call instance methods can be marked as static (Shared in Visual Basic). After you mark the methods as static, the compiler will emit nonvirtual call sites to these members. Emitting nonvirtual call sites will prevent a check at run time for each call that makes sure that the current object pointer is non-null. This can achieve a measurable performance gain for performance-sensitive code. In some cases, the failure to access the current object instance represents a correctness issue.

So to summarize it: it's perfectly fine as it is.

Reading all the additional comments: it seems the real problem is that an injected repository already exists and the whole method should scrapped and replaced by the line claim = this.injectedClaimsRepository.GetClaimDetails(claim.ClaimNumber);. But that's a whole other problem. The keyword static is perfectly fine if used correctly, the posted code did not show it wasn't.

Ok, thank you! I am learning here. So in general, your answer makes sense. Now... in this specific case, the developer is (re) creating a repository that has been injected into the controller. While this works, wouldn't it make more sense to make the method non-static and use the injected repository? — Casey Crookston, Feb 13 '20 at 16:27
@CaseyCrookston Indeed, I would say the whole method should be deleted if there already exists an injected repository. — nvoigt, Feb 13 '20 at 16:29
Thanks! After your update, I'm marking this as the answer, as it covers the question in general and the specific instance. Much appreciated! — Casey Crookston, Feb 13 '20 at 16:33

Is there ever a valid reason to have a static method inside a non-static class?

2 Answers2