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.

Community
  • 1
AncientSwordRage
  • 1,033
  • 1
  • 11
  • 24
  • 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. – AncientSwordRage 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
  • @Telastyn then what should I do? Completely rewrite? – AncientSwordRage Jan 12 '14 at 18:37
  • it would help to use real code and explain what you want to tackle. What you have now is kind of mumbo-jumbo. – Wim Ombelets Jan 12 '14 at 19:21
  • @WimOmbelets I'd love to but because of workplace policies I can't post actual code from the project. If I find myself with lots of time I might able to write up a SSCCE, but I'm not sure. – AncientSwordRage Jan 12 '14 at 19:31
  • 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
  • @WimOmbelets I've had a stab at it. Yeah some of the previous code has been split out using strategies by a colleague, but he's no longer working on this code base. He's not touched the Global State Class, though I know he intended to at some point, and I'm trying to replicate his actions. But if we have 3 kinds of report and 2 (more in the future) ways to export said report, that would lead to 9 classes (3 concrete instances of report, and then 6 instances for each Report+Export combination) which seems bloated to me. – AncientSwordRage Jan 12 '14 at 19:51
  • whoa wait! why would your Report include it's particular way to export it? What if you have a Graph class that also needs to be exported using the same export code? Keep it DRY. No No, not 9 classes but x report types (template / factory) and y ExportStrategy implementations (strategy) seems to make a lot more sense. – Wim Ombelets Jan 12 '14 at 20:17
  • and don't emulate your predecessor's style just because. He was not necessarily right (or wrong). – Wim Ombelets Jan 12 '14 at 20:19
  • let us [continue this discussion in chat](http://chat.stackexchange.com/rooms/12451/discussion-between-pureferret-and-wim-ombelets) – AncientSwordRage Jan 12 '14 at 20:21
  • 1
    [Tell, don't ask](http://programmers.stackexchange.com/a/216681/31260 "'your objects expose data instead of behavior'"): `myPojo.whatKindOfReport()==Reports.reportTypes` => `myPojo.isKindOf(Reports.reportTypes)` or `myPojo.hasKindOf(Reports.reportTypes)` – gnat Jan 12 '14 at 23:26

2 Answers2

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.

Euphoric
  • 36,735
  • 6
  • 78
  • 110
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.

Idan Arye
  • 12,032
  • 31
  • 40
  • This sounds...promising. Is this an established 'pattern' or just good coding? – AncientSwordRage 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