2

I'm trying to refactor some code, and one of the major changes is to remove the (ab)use of static classes to give global state.

I've tried to split up some of the 'global state' functionality with POJOs that can be passed around.

But the further down the rabbit hole I go the more and more I feel like this is an Anti-Pattern.

Is holding state in POJOs and then passing them around bad form? It means the outline of my methods often look like this:

public MyPojo makeReport(int myInt, String pathToInput, MyPojo myPojo){
    ...
    if(myPojo.whatKindOfReport()==Reports.reportTypes){
        ...
    }
    ...
    myPojo.isWritingReportToFileSuccessful(true);
    ....
    return myPojo;
}

Where the POJO 'state' is set from the GUI that the user interacts with. For instance with tickboxes for report types etc.

But this blocks other return types, and seems overly complicated.


I've taken this approach because the application has several 'paths' (see whatAmIDoing) the user can take in running it, each of which has several stages. What options the user has depends heavily on what path they pick and then from there, the result of that choice (normally, success or failure, see myPojo.isSomethingSuccessful(true)). I'm trying to encode that information in a POJO rather than a Global State Class.

Is this an anti-pattern? How can I get round this?


Edit: I think what I'm after is something like Chain of Responsibility. After looking at this answer I'm not sure.

12
  • It's certainly not good, but without knowing what's actually going on, we are probably not going to be able to suggest how to improve things.
    – Telastyn
    Commented Jan 12, 2014 at 18:16
  • @Telastyn I hope my edit makes things clearer. If not, let me know. Commented Jan 12, 2014 at 18:22
  • Not really. The method depending on some passed in state rather than some internal state is beyond smelly.
    – Telastyn
    Commented Jan 12, 2014 at 18:30
  • 1
    you can still replace the meaningless class names with comparable concepts and add a story that, although hypothetical, could provide insight into what you're trying to do. When you say "depending on a path the user picks" I'm thinking Strategy, State, DI, ... but we're all still pretty much guessing here. You're wasting time right now on a post that isn't getting you anywhere but more comments. Take the time to make the SSCCE ;-) Commented Jan 12, 2014 at 19:39
  • 1
    Tell, don't ask: myPojo.whatKindOfReport()==Reports.reportTypes => myPojo.isKindOf(Reports.reportTypes) or myPojo.hasKindOf(Reports.reportTypes)
    – gnat
    Commented Jan 12, 2014 at 23:26

2 Answers 2

2

This looks as if the doSomething method should be member of MyPojo class. Getting object as a parameter and then returning it? Definitely doesn't look good.

The way you describe your code seems to be even more indicative of some kind of class structure. Using enum as descriptor to what is done is clear smell that should be replaced with class hierarchy where each class represents one path the code can take. As described here.

And as Idan said, making the object a field of class that contains the doSomething method instead of parameter to the method itself can be better. Also, you can make the object as constructor parameter, so you can use dependency injection.

1

Stop thinking in terms of operations and start thinking in terms of data.

doSomething is not a static method, therefore it belongs to an object which can hold state - lets call the class of that object Foo, and say you have created an instance named foo.

Now, instead of saying that foo.doSomething needs an instance of MyPojo, you should say that foo needs to do something with MyPojo. So - you can do foo.setMyPojo(myPojo); and then you can call foo.doSomething without the MyPojo argument - because it already has it. In addition, when you call other methods of foo it already has myPojo set! And the best thing - this allows you to draw a graph of dependencies based on objects, not methods.

2
  • This sounds...promising. Is this an established 'pattern' or just good coding? Commented Jan 12, 2014 at 18:17
  • 2
    Nothing with a fancy name - simply following the basic OOP rule that state should be organized in objects. Your method follows the functional rules that function calls should only depend on their arguments(and the function itself ofcourse) and that their result should only be expressed by their returned value - but that style doesn't work very well when you don't keep a high percentage of immutability, so you are better off with the OOP style, that lets you keep the existing state-changing flow but organize the state better.
    – Idan Arye
    Commented Jan 12, 2014 at 23:01

Your Answer

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

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