Click here to Skip to main content
15,617,112 members
Articles / Programming Languages / C#
Tip/Trick
Posted 11 Feb 2018

Tagged as

Stats

19K views
8 bookmarked

"Method can be made static" May Hide OO Design Flaw

Rate me:
Please Sign up or sign in to vote.
4.95/5 (19 votes)
16 Aug 2022CPOL2 min read
"Method can be static" Microsoft Code Analysis warning may hide a violation of object-oriented design

Introduction

Wandering through codebases, I've encountered some examples of code where Microsoft Code Analysis issues the above-mentioned warning. Although the fix seems straightforward, the warning itself may hide a more subtle issue connected to object responsibility assignment.

Toy Example

Let's take a look at the following code:

C#
public class EmailConstructor
    {
        private const string Signature = "Regards";

        public string Construct(User recipient, string body)
        {
            var builder = new StringBuilder();
            builder.Append($"Hello {GetNiceUserName(recipient)}");
            builder.Append(Environment.NewLine);
            builder.Append(body);
            builder.Append(Environment.NewLine);
            builder.Append(Signature);
            return builder.ToString();
        }

        private string GetNiceUserName(User user)
        {
            return $"{user.Name} {user.Surname}";
        }
    }

    public class User
    {
        public string Name { get; set; }
        public string Surname { get; set; }
    }

Indeed code analysis issues the warning.

Image 1

Feature Envy

Although things may seem obvious at first glance, actually the code above is the example of Feature envy. As the rule of thumb which would help you to spot such cases without using tools such as ReSharper, you may use one of the principles postulated by Craig Larman in his book "Applying UML and patterns".

Quote:

Information expert will lead to placing the responsibility on the class with the most information required to fulfill it.

According to this principle, you can fix the warning my moving method inside the User class as follows as the User is the one who has more information to represent his name in one or another way:

C#
public class EmailConstructor
    {
        private const string Signature = "Regards";

        public string Construct(User recipient, string body)
        {
            var builder = new StringBuilder();
            builder.Append($"Hello {recipient.GetNiceUserName()}");
            builder.Append(Environment.NewLine);
            builder.Append(body);
            builder.Append(Environment.NewLine);
            builder.Append(Signature);
            return builder.ToString();
        }
    }

    public class User
    {
        public string Name { get; set; }
        public string Surname { get; set; }

        public string GetNiceUserName()
        {
            return $"{Name} {Surname}";
        }
    }

High Cohesion

Naturally, the question arises from the example above: "Won't this lead to bloating User class with lots of responsibilities such as, for example, working with persistent storage?". The answer is that Information expert principle should be used together with high cohesion principle.

Quote:

High cohesion is generally used in support of low coupling. High cohesion means that the responsibilities of a given element are strongly related and highly focused. Breaking programs into classes and subsystems is an example of activities that increase the cohesive properties of a system. Alternatively, low cohesion is a situation in which a given element has too many unrelated responsibilities. Elements with low cohesion often suffer from being hard to comprehend, hard to reuse, hard to maintain and averse to change.

Thus if we, for example, would like to extend our toy code with persistent storage interaction, we would likely extract highly cohesive functions dedicated to that into some sort of Unit of Work pattern.

Conclusion

Static members have some disadvantages such as complicating unit testing. That's why static should be considered with care. Craig Larman's GRASP patterns in combination with SOLID allow us to examine such cases in order to reduce the support cost of our codebases.

History

  • 11th February, 2018: Initial version
  • 16th August, 2022: Replaced ReSharper with Microsoft Code Analysis

License

This article, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)


Written By
Team Leader
Ukraine Ukraine
Team leader with 8 years of experience in the industry. Applying interest to a various range of topics such as .NET, Go, Typescript and software architecture.

Comments and Discussions

 
PraiseIt fits Functional better than OO Pin
mldisibio19-Aug-22 7:37
mldisibio19-Aug-22 7:37 
GeneralRe: It fits Functional better than OO Pin
PIEBALDconsult19-Aug-22 18:31
professionalPIEBALDconsult19-Aug-22 18:31 
GeneralRe: It fits Functional better than OO Pin
mldisibio20-Aug-22 7:35
mldisibio20-Aug-22 7:35 
GeneralRe: It fits Functional better than OO Pin
PIEBALDconsult20-Aug-22 8:36
professionalPIEBALDconsult20-Aug-22 8:36 
QuestionObject oriented Pin
Member 1452198417-Aug-22 7:38
Member 1452198417-Aug-22 7:38 
AnswerRe: Object oriented Pin
John Brett17-Aug-22 23:56
John Brett17-Aug-22 23:56 
AnswerRe: Object oriented Pin
Bohdan Stupak5-Sep-22 0:26
professionalBohdan Stupak5-Sep-22 0:26 
QuestionWell no again Pin
carloscs17-Aug-22 6:17
carloscs17-Aug-22 6:17 
AnswerRe: Well no again Pin
Dmitry Mukalov17-Aug-22 19:05
Dmitry Mukalov17-Aug-22 19:05 
QuestionIt might be an artifact of this example, but I respectfully disagree. Pin
dchalom16-Aug-22 7:09
dchalom16-Aug-22 7:09 
AnswerRe: It might be an artifact of this example, but I respectfully disagree. Pin
Bohdan Stupak5-Sep-22 0:21
professionalBohdan Stupak5-Sep-22 0:21 
QuestionReSharper is not a holy cow... Pin
Oleg Shilo23-Feb-19 23:32
Oleg Shilo23-Feb-19 23:32 

I'm not in a position to blame great tool that helps millions of developers and personally me for that.


Bohdan, I challenge you here. Smile | :)
You and everyone else are in a position to disagree with any behavior of any product.

There is nothing wrong in reflecting on even the greatest tool of all. ReSharper is indeed great but as any other product it exhibits some limitations and shortcomings.

I don't want to make this post about ReSharper even though I have seen an extreme damage this excellent tool inflicted when it was used blindly. My post is about not feeling good about seeing you almost apologizing Smile | :)

The most direct expression of your message about 'static' (and ReSharper for that matter) was given in your conclusion "That's why static should be considered with care"

Amen to that.
AnswerRe: ReSharper is not a holy cow... Pin
Bohdan Stupak16-Aug-22 5:29
professionalBohdan Stupak16-Aug-22 5:29 
PraiseGood point Pin
wmjordan11-Feb-18 13:43
professionalwmjordan11-Feb-18 13:43 
GeneralRe: Good point Pin
Bohdan Stupak13-Feb-18 21:57
professionalBohdan Stupak13-Feb-18 21:57 
AnswerWell, no PinPopular
pt140111-Feb-18 4:12
pt140111-Feb-18 4:12 
GeneralRe: Well, no Pin
Bohdan Stupak11-Feb-18 4:35
professionalBohdan Stupak11-Feb-18 4:35 
GeneralRe: Well, no Pin
pt140111-Feb-18 5:09
pt140111-Feb-18 5:09 
GeneralRe: Well, no Pin
Bohdan Stupak11-Feb-18 5:21
professionalBohdan Stupak11-Feb-18 5:21 

General General    News News    Suggestion Suggestion    Question Question    Bug Bug    Answer Answer    Joke Joke    Praise Praise    Rant Rant    Admin Admin   

Use Ctrl+Left/Right to switch messages, Ctrl+Up/Down to switch threads, Ctrl+Shift+Left/Right to switch pages.