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.

  • 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 Jan 12 '14 at 18:16
  • @Telastyn I hope my edit makes things clearer. If not, let me know. – Pureferret Jan 12 '14 at 18:22
  • Not really. The method depending on some passed in state rather than some internal state is beyond smelly. – Telastyn Jan 12 '14 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 ;-) – Wim Ombelets Jan 12 '14 at 19:39
  • 1
    Tell, don't ask: myPojo.whatKindOfReport()==Reports.reportTypes => myPojo.isKindOf(Reports.reportTypes) or myPojo.hasKindOf(Reports.reportTypes) – gnat Jan 12 '14 at 23:26
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.

| improve this answer | |
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.

| improve this answer | |
  • This sounds...promising. Is this an established 'pattern' or just good coding? – Pureferret Jan 12 '14 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 Jan 12 '14 at 23:01

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.