Is this code structure beneficial in any way?

Question

I was recently thrown into a Java web application project, and I’ve come across a number of classes that follow this type of format:

public class MyThingy {
   private final int p1;
   private final String p2;
   …

   public MyThingy (int p1, String p2, …) {
      this.p1 = p1;
      this.p2 = p2;
      …
   }

   public static void doSomething(int p1, String p2, …) throws Throwable     {
      final MyThingy myThingy = new MyThingy(p1, p2, …);
      myThingy.execute();
   }

   private void execute() throws Throwable {
      //do stuff 
   }
}

It seems like this could be accomplished with the following code, which to me seems way easier to read.

public class MyThingy {

   public static void doSomething (int p1, String p2, …) throws Throwable {
      //do stuff 
   }

}

The only possible benefit I can see from doing it the first way, is that if you had to break up execute() into smaller pieces, they could all share the initial parameters without having to explicitly pass them around. But this maybe only benefits the lazy coder, as it becomes difficult for the reader to tell which methods need which parameters and when the values might be changed (akin to global variables.)

Is there something I'm missing? Threading, performance?

Edit: I should have mentioned, although the constructor is public, it is not called. The only usage is like this:

MyThingy.doSomething(p1, p2...);

Aside from this in itself being problematic for testing, I can't see any reason not to put the logic of execute() directly into doSomething(). Even if we were to get rid of the static function, the constructor still doesn't make sense to me; I think the parameters should be passed directly to the method that will use them.

Answer 1

I think your intuition on this issue is right.

• What we have here is a plain (in this case, static) method whose implementation has been "broken up" into a class.
• The "method as a class" technique reduces the number of variables you have to explicitly pass around, while in fact obscuring how data flows using what are, in spirit, "global" variables.
• None of this is actually in the spirit of OO, since the instance is throwaway and its state is ephemeral.
• And yet, this is a common pattern, especially in introductory Java books and may have been on the minds of Java designers (who introduced lots of ways to declare classes, seemingly to aid this usage).

So, should you use it?

• Sure, sometimes.

In fact, at issue is not just pure "methods as classes." In normal classes, you may be tempted to create fields that don't hold state in-between invocations of public methods. They're used only to avoid passing parameters between private methods. In the past, I tried to avoid it at all cost, but then realized that long parameter lists suck too.

I try to avoid pure "method classes" as I think they come with a lot of mental baggage (so much potential potential). (I also lean against disposable fields in regular classes.) But if there's a ton of variables to pass around, the win in code cleanliness may be worth it.

I try to use classes when there is a point to it, and in fact it's usually easy to imagine one. Perhaps in the future you'll extend the class with additional implementations. Perhaps the instance can be thought of as a function object that you'd want to initialize and pass around. And then, the class is no longer a pseudo-OO "method class."

Answer 2

The extra indirection through the static method call separates the concerns of how an object is created from the code that uses the object. What you have here is something very similar to a simple factory method (it does not actually return the object, but the indirection of object creation is the same).

This can be useful if you need to control the type of object that is created. In this simple example there is no decision to be made, but it would be easy to add that logic.

What the static code means to callers is "I need to do something with this state but don't know how to delegate." The static method can then delegate to the proper implementation based on the object provided.

Perhaps when running in a unit test there is a mock object being used. Maybe based on the parameters a different object could be swapped in that implements a different algorithm for doing whatever it needs to do. By localizing that code to one location, the decision can be made in one place rather than arbitrarily many.

---

Given how your question is worded and the fact that no construction decision is being made in the static methods, I have a feeling this might just be a case of over-engineering. Most code does not need to delegate to a factory. The fact that you keep encountering code like this that does not make any decisions as I would expect a static factory-like method to do tells me someone read the GoF book and ran amok.

Answer 3

Well, if your second example (showing the complete code) looks like this:

public static void doSomething(int p1, String p2, …) throws Throwable     {
      final MyThingy myThingy = new MyThingy(p1, p2, …);
      myThingy.execute();
   }

then you're still going to need the constructor

public MyThingy (int p1, String p2, …) {
      this.p1 = p1;
      this.p2 = p2;
      …
   }

which in turn sets

   private final int p1;
   private final String p2;

and now you're back where you started.

You could, of course, just eliminate the constructor and go with getters and setters instead

private final int p1;
private final String p2;

public void SetP1(int p1)
{
    this.p1 = p1;
}

public int GetP1()
{
    return p1;
}
// repeat for p2

...but this will complicate your static method call, as you will now need several lines of code to call the setters first before calling your static method, where a single line making use of the constructor call would have sufficed.

Of course, this only works if your member variables are not final. Therefore, you're still going to need the constructor.

Answer 4

Usually a class has lots of instance methods, and once the object is created, the implementation of execute can take advantage of all these instance methods. If you refuse to create an object, all these instance methods are useless.