0

I have to create custom buttons but in order to encapsulate the logic of button creation I've this class (VB.net):

Public Class ButtonCreator

    Public Shared Function CreateBaseButton(Optional label As String = "") As Button
        Dim btn As New Button()
        btn.Text = label
        btn.Size = New Size(100, 25)
        btn.Anchor = CType((AnchorStyles.Top Or AnchorStyles.Bottom), AnchorStyles)
        Return btn
    End Function

    Public Shared Function CreateClickableButton(clickHandler As EventHandler, Optional label As String = "") As Button
        Dim btn As Button = CreateBaseButton(label)
        AddHandler btn.Click, clickHandler
        Return btn
    End Function

    Private Shared Function CreateCloseButton(result As DialogResult, Optional label As String = "")
        Dim btn As Button = CreateClickableButton((Sub(o, e)
                                                       o.FindForm().DialogResult = result
                                                       o.FindForm().Close()
                                                   End Sub), label)
        Return btn
    End Function

    Public Shared Function CreateContinueButton(Optional label As String = "")
        Return CreateCloseButton(DialogResult.Yes, label)
    End Function

    Public Shared Function CreateStopButton(Optional label As String = "")
        Return CreateCloseButton(DialogResult.No, label)
    End Function
End Class

Other classes can call these method to create very basic buttons and add them to different panel containers.

I've tried to make the methods as cohesive as possible but I'm still uncomfortable with my solution, and I don't know why. I suspect that this looks akward because I have a static class full of static methods that could be somehow refactored, but I can't see any other way to achieve a better design.

Is it possible that this is a code smell? (I couldn't find anything online) What are the possible solutions to this?

Improve this question

6 Answers 6

Reset to default
2

One way to look at this, would be to look at the specifics of what you are trying to do - in this case creating things - that would push you towards using a factory pattern instead.

I tend to be a bit more pragmatic and leave the code as it is until I need something more complex. The one thing I would keep an eye out for there is testing - specifically ask yourself if it would help your testing, if you were to change this code.

In answer to the more generic question (the title):

Is a static class full of static methods a code smell?

If none of these methods mutate state and none have side effects, it could be argued that you are using a "functional style" of programming.

You could extend this slightly and say its also fine to mutate state (as your example code does) as long as the state you are manipulating is passed in to the function. As such I would be tempted to defend it that way and say it's fine.

The main counter-argument against this logic is: Unit Testing

In that other code will find it difficult to de-couple themselves from these methods in that the "advantage" of static methods is you don't have to do any special wiring to access them.

IMO The testing argument becomes stronger as the methods become more complex. The methods you have above could be implemented as a pre-processor alias / inline in other languages, so I just don't give the testing argument sufficient weight for such simple code.

Improve this answer
10
  • 1
    I like this answer except one little niggle: the static class is procedural, not functional. Otherwise a good explanation. Commented May 12, 2023 at 0:49
  • 2
    @GregBurghardt: I am inclined to say this code is too simple to distinguish whether it is designed in a procedural or functional way. Commented May 12, 2023 at 4:23
  • I have made an edit to clarify that the code as written does mutate state - so a purist would not consider it to be functional. However even though the code mutates state (specifically registers a callback in CreateCloseButton) I am still fine with it. Commented May 12, 2023 at 13:33
  • "leave the code as it is" - you mean it is fine that ContinueButton is based on the CloseButton? 😉 Commented May 12, 2023 at 15:45
  • @mentallurg: the fact that ContinueButton calls CloseButton is an implementation detail that callers do not need to know about. It looks like they are functionally the same (they close stuff), so the abstraction is not broken. Commented May 12, 2023 at 15:58
2

In general, what you've described is a Factory - a class which serves to instantiate other classes.

In some (many?) cases, factories have all static methods, though it is possible to refactor it so everything is not static. You would then need to create an instance of the Factory class before using it, of course. There are a couple of considerations on this approach:

  1. Are you re-using the factory object?
  2. Do you need to inject the factory as a dependency somewhere?
  3. Do you need to mock the factory for testing?

If any of those are true, you may want to use a non-static class that you instantiate.

Improve this answer
2
  • 3
    Reuse is not the only argument pro non-static factory. Injectability and mockability are two other arguments. Commented May 12, 2023 at 1:11
  • @Flater good points, updated my answer to reflect those scenarios Commented May 12, 2023 at 14:01
1

Consider defining a class for each type of button. Then your will not need these static methods at all.

  • Put the initialization logic into constructors.
  • A class should encapsulate some concept. As long as you don't need both clickable and not clickable buttons, don't define 2 classes, a single class is sufficient. Means, put the logic of methods CreateBaseButton and CreateClickableButton to a single class, ClickableButton.
  • Pay attention to inheritance. Is ContinueButton a subtype of CloseButton? "Continue" and "Close" are completely different things. That's why ContinueButton should not be a subtype of CloseButton. Instead, both should be subtypes of ClickableButton. Same about StopButton: It should be a subclass of ClickableButton.

Thus, the hierarchy would be as follows:

ClickableButton
+--ContinueButton
+--CloseButton
+--StopButton

Is a static class full of static methods a code smell?

It depends on the purpose of the methods. For instance, if the methods provide functionality that is hard to associate with some object, then it is fine to put it to static methods. Normally, utility classes consist of static methods.

Now to your code. You chain many methods for unclear purpose. May be you wanted to avoid duplication. But how about readability? For instance, to understand what CreateContinueButton does, one has to look at CreateContinueButton, then CreateCloseButton, then CreateClickableButton, then CreateBaseButton. And, for instance, to understand what handler is used, one has to traverse the whole chain to find at what place the handler is defined and what it is doing, then switch back. These are just a few lines, but the effort to understand them can be unproportionally high.

Improve this answer
6
  • To understand CreateContinueButton all you need is to read its code and trust that everything called does what it is supposed to do. Would you prefer duplicating code again and again and again? Commented May 12, 2023 at 14:00
  • @gnasher729: In the current form, other developers will need to analyze 4 (!!) methods that all together consist of less than 10 lines. If one decides not to use classes, as I suggested, but keep static methods, then of course duplication is better in this case. I see time to time questions here when authors operate with patter names and buzzwords without thinking about the sense. Commented May 12, 2023 at 15:58
  • 1
    Unless the different kinds of buttons behave differently, there is no need to create a class hierarchy. Each class does not provide a specialization other than button text, but this solution would work. And I've certainly done worse with better technology. Commented May 12, 2023 at 16:02
  • @gnasher729: When we talk about duplication, we should consider pros and cons. What are the pros in this particular case? 5 lines of code. What are the cons? Essentially worse readability. Developers will need more cognitive efforts to understand the current logic compared to the case when all these methods would be isolated. Code reuse in this particular case gives no advantages. Thus, duplication is this case is definitely better than chaining 4 methods each contributing a single line. If methods had say 30 - 50 lines, that would be another story. Commented May 12, 2023 at 16:03
  • @gnasher729: 1) They do behave differently by returning different result. 2) Behaviour is not the only factor to consider about class. The initialization logic, depending on the context, can also be a reason to define a class. The class encapsulates initialization logic. This simplifies the usage of such class. Instead of thinking what values to set to each of N parameters, developers just pays attention to the name of the class. This reduces the number of bugs and reduces the efforts... Commented May 12, 2023 at 16:11
0

Thanks to everyone for responding.

The Factory Method that @mmathis proposed was indeed close to my original design, but it didn't work, because I wasn't using different Factories anywhere else, so the static methods weren't going to dissapear just like that. I'll leave a link anyway for anyone interested Factory Method (in fact there's an almost identical example)

I took the @mentallurg approach instead and refactored the static class into two hierarchical classes, resulting in a much cleaner solution:

Public Class ClickableButton
        Inherits Button

        Public Sub New(click As EventHandler, Optional label As String = "")
            MyBase.New()
            AddHandler Me.Click, click
            Me.Text = label
        End Sub

End Class

Public Class CloseDialogButton
        Inherits ClickableButton
        Public Sub New(result As DialogResult, Optional label As String = "")
            MyBase.New(Sub(o, e)
                           o.FindForm().DialogResult = result
                           o.FindForm().Close()
                       End Sub, label)
        End Sub
End Class

The static methods are gone, the code readability is widely improved and logic repetition is avoided. The creation of Continue, Ok and Stop buttons are fully covered by CloseDialogButton and its parameters (The size and anchors could be assigned later as these are very specific). However, this solution fires the one-bullet gun, inheritance, which is known to be a very non-flexible solution because it is the strongest link possible between classes.

It turned out to be the most dummy and basic alternative, which I somehow didn't see. While I couldn't find anything better for this case, there may well be other cases where different approaches could be taken.

Improve this answer
0

No. It isn't.

In an OO language, it's an appropriate way to code pure functions. Both Java and .Net do it with the Math class.

In your case, you're using it for factories. A static method is a common choice for a factory.

However. It seems to me that your real question is, “how do I best organise and encapsulate code for creating specific kinds of buttons?”

For which your subclasses seem much better than a set of factory methods, because it fits much better with the style of how the UI framework creates buttons and other UI elements.

Improve this answer
0

Yes, it's totally a code smell.

You have two essential problem over and above general ones with all static methods.

  1. The object becomes simply a namespace for a random group of unconnected functions.

    eg ButtonCreator.CreateBellyButton(Mammal animal) the only connection is the word "Button".

    Or someone creates a second similar MyButtonCreator because they don't know about your collection and now you have two

    The list of functions that fit the "ButtonCreator" group definition turns out to be larger than you would like to put in a single file.

  2. The static functions start to call each other (which i see you have already done!). Now you have a hidden dependency, or perhaps a weirdly setup singleton object.

These kinds of problem are non-obvious from the initial, small, class you write but can cause problems down the line.

They are both solved by simply make the class and methods non static

Improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.