8

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.

  • I don't suppose the company standard says anything about avoiding static functions? Alternatively, what kind of automated testing is being done with this code? Mocking becomes almost impossible when static functions get introduced. – KChaloux Nov 4 '15 at 21:26
  • 3
    Just as a note: throws Throwable is a bad practice, it should be at least throws Exception, or something more specific, if possible. And, since bad practices usually come together, I'd say that this code template is just another bad practice. – scriptin Nov 4 '15 at 21:29
  • The static method is just a helper to perform new(...) + execute() in one call. – Kasey Speakman Nov 4 '15 at 21:32
  • You could turn it either way: this is an ugly solution. It would be better to separate object creation from consumption. – Thomas Junk Nov 4 '15 at 21:35
  • 1
    Also note that the author obviously intended for the constructed object to be immutable (data is final). The static method is the only API into the object since all members are private. In just the code you've shared, I don't see any benefits to this over making execute a static method taking p1, p2, .... – Kasey Speakman Nov 4 '15 at 21:44
2

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

  • 1
    I like what you're suggesting in the last paragraph here. If the execute() method were public, I could create (and initialize) the object, then have something execute it later. That might make sense, and would also improve testability. In that case it's actually the static function that is extraneous. – Mishelle Nov 5 '15 at 15:56
  • This is the answer this question has been waiting for. +1 – cbojar Nov 5 '15 at 19:31
6

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.

  • 1
    Where do you see a Factory method? None of the OP's examples returns anything. – Robert Harvey Nov 4 '15 at 22:34
  • @RobertHarvey: It's effectively a wrapped factory method. Just because it's not exposed to the callsite doesn't mean it's not a factory! – Lightness Races in Orbit Nov 4 '15 at 22:36
  • 1
    @RobertHarvey I think that is an issue of word choice. It behaves similarly to a factory method, but does not actually return anything. Then again, I never claimed it did, only that it delegated, which is still an accurate statement. – user22815 Nov 4 '15 at 22:38
  • I agree with @RobertHarvey. An object constructed and used entirely within a method is an implementation detail, not a factory product. – cbojar Nov 5 '15 at 1:40
  • Interesting. I modified the terminology in my answer, but the point I made is identical. Weird how that happened... – user22815 Nov 5 '15 at 1:46
0

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.

  • It doesn't make sense to have a public setter on a final variable. It can only be initialized once. – Kasey Speakman Nov 4 '15 at 21:33
  • 1
    @KaseySpeakman: Ergo, you still need a constructor. – Robert Harvey Nov 4 '15 at 21:34
  • I'm not entirely sure that this answer speaks to the question... – cbojar Nov 5 '15 at 1:38
  • Whats more, its not valid (has a compile error): ideone.com/0nDRgY --- the final value must be able to be proven to not be settable anywhere else. Having a setter makes that impossible, and thus a compile error. – user40980 Nov 5 '15 at 1:54
  • @MichaelT: I said that in my answer already (in the last paragraph). – Robert Harvey Nov 5 '15 at 2:31
0

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.

  • I think the OP is saying that there are no other instance methods beside execute. Thus asking why not inline the whole thing. – cbojar Nov 5 '15 at 1:37
  • Yes, that is exactly my question! – Mishelle Nov 5 '15 at 15:47

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.