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?

  • When shuffling the array, is a new array returned or is the existing array shuffled? – Greg Burghardt Apr 5 at 16:53
  • The existing one is shuffled. Curious to see what difference it makes here. :) – Sid Apr 5 at 17:02
  • Where does the array come from? What class or piece of code is responsible for creating it? – John Wu Apr 5 at 21:39
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.

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 ...

  • I'm curious to know why you would need a FakeShuffler for testing. If the shuffling/randomness of the collection is the thing you are testing in Widget, you are basically testing the shuffling algorithm. The algorithm, when in object form or static, should have its own tests. – Greg Burghardt Apr 5 at 18:02
  • @GregBurghardt It's not uncommon, from what I've seen, to need to provide fake randomness (or shuffling) to validate that your domain object is actually invoking a shuffle and that the resulting math, collection, state, etc. is as expected for the permutation. – svidgen Apr 5 at 18:21
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.

  • 1
    I could be wrong, but, I think you're reading too far into the question ... If the OP said they were loading a List of Widget with Widget specifically (don't think it said that), I'm really not sure it's relevant ... What if Widget is a TreeNode of some sort? – svidgen Apr 5 at 18:26
  • 1
    Or perhaps Widget is AdvertisementPanel and the List of things to be shuffled is a List of AdBoxes or something ... I dunno. Either way, I think you're critiquing code you haven't actually seen instead of answering the question here. – svidgen Apr 5 at 18:30
  • @svidgen is correct; you don't know what the Widget class and loadWidget method do. – Sid Apr 5 at 18:38
  • @Sid "... loadWidget. This method needs to shuffle an array" Silly me for assuming a method named loadWidget that shuffles would shuffle widgets. Would you mind editing the question to make the responsibilities clearer? – candied_orange Apr 5 at 18:42
  • 1
    @sid: The names "Widget" and "loadWidget" can imply some sort of behavior or responsibility. I think what candied_orange is trying to get at is what a Widget does, and what loadWidget does can be very relevant to this question. It may indicate you have another "thing" that can be modeled as its own class. – Greg Burghardt Apr 5 at 19:32
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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.