1

My Widget class has a method called loadWidget. This method needs to shuffle an array to work properly, among other things. In Widget, I could implement a Fisher-Yates shuffle as its own static method named shuffle and then call shuffle in loadWidget. However, this looks unseemly to me for two reasons:

  1. The shuffling algorithm has little to do with Widget and putting many unrelated algorithms there makes the class harder to read and understand.

  2. In the future, I may want to use the Fisher-Yates shuffle in other classes, but it feels like an anti-pattern to call Widget.shuffle whenever I want to do that.

So the solution could be to create a class of support methods. I could call SupportMethods.shuffle inside Widget and other classes whenever I want to shuffle an array. Is this a good practice or does it violate design principles?

Sid
  • 176

4 Answers4

2

I'm always leery of static methods that have side effects (e.g. modifying data outside its own function call). Since the logic of the shuffling algorithm is likely closely intertwined with the code itself, you are probably unable to extract parts of the algorithm into helper functions.

I would recommend making the static method free of side effects by returning a shuffled copy of the original array. Depending on the array size and performance requirements this might not be desirable — but I would lean towards this approach unless you have a measured performance problem.

And even then, it won't hurt to have two copies of the shuffling algorithm around. One can return a shuffled copy of the array, and another can shuffle the existing array. Then you can have a version free of side effects, and another version called on a case-by-case basis that does have side affects. You can probably call one from the other:

// Modifies the existing array
SupportMethods.shuffle(array)

// Copies the array, then calls SupportMethods.shuffle(copyOfArray) and returns copyOfArray
SupportMethods.copyAndShuffle(array)

Be sure to name them differently and provide some additional hints in the documentation of this API.

Greg Burghardt
  • 45,304
1

If you need a specific kind of shuffling, your basic intuition is probably right.

Assuming your Widget's "single responsibility" isn't "shuffling", it shouldn't know much about shuffling. A Shuffler class may be a good idea.

But, I would really double-check your language and it's standard libraries first. Make sure you're not overlooking a suitable shuffling or "randomizing" implementation there first.

If you do need (or really want) to implement your own shuffling, I strongly suggest making a Shuffler base class. When applicable, make it injectable into any class that needs shuffling so you can swap out shuffling algorithms and inject a FakeShuffler during testing ...

svidgen
  • 15,252
1

Why does a widget know about loading widgets? Why is it handling arrays of widgets?

This feels like your asking a book to do what a bookshelf does. Not very good semantics.

I'm in favor of moving this stuff out to SuportingMethods except that is a terrible name. Call it WidgetRack or ArrayList<Widget> or really any name that makes clear what does and doesn't belong in it. SuportingMethods is about as good a class name as Misc. You've created a junk drawer.

With that broken out you could break it down further so WidgetRack doesn't have to know exactly what sort implementation it uses. But in any case let the Widget focus on being a Widget. Not on everything that happens to Widgets.

candied_orange
  • 119,268
1

It's difficult to discuss problems like these with generic names like Widget. Usually, a class you are tempted to name something like Utilities or SupportMethods is going to end up not very cohesive, but there are other groupings that might work for you.

If your language has extension methods, you can group functions like shuffle together with other extension methods on arrays. You could group it in a Random class with other related methods. Another option is to create a WidgetLoader or WidgetFactory class and put all the methods related to loading but not using a Widget in there, splitting off shuffle into a different class only if future circumstances dictate.

The exact reason methods are grouped together doesn't matter nearly as much as the fact that a common reason exists and is easy to articulate.

Karl Bielefeldt
  • 148,830