How to deal with monstrous Struts Actions?

Question

I inherited this gigantic legacy Java web app using Struts 1.2.4. I have a specific question regarding Actions. Most of the pages have exactly one Action, and the processExecute() methods are hideous monsters (very long and tons of nested if statements based on request parameters).

Given that Actions are an implementation of the command pattern, I'm thinking to split these Actions into one Action per user gesture. This will be a large refactoring though, and I'm wondering:

1. Is this the right direction?
2. Is there an intermediate step I could take, a pattern that deals with the mess inside the monolithic actions? Maybe another command pattern inside the Action?

Tog

Answer 1

My way of dealing with this would be:

• dont do 'everything at once'
• whenever you change anything, leave it better than you found it
  • replacing conditionals with separate Action implementations is one step.
  • Better yet: Make your implementations separate from the Action classes so that you can use it when you change frameworks
  • Keep your new Command implementation absolutely without references to Struts, use your new Actions as Wrapper around these implementations.
  • You might need to provide interfaces to your Struts ActionForms in order to pass them around without copying all the data. On the other hand - you might want to pass around other objects than ActionForms that are usually a bunch of Strings (see your other question about Struts 1.2 ActionForms)
• start migrating parts to newer & better technology. Struts 1.2 was great when it came out, but is definitely not what you want to support in eternity. There are some generations of better frameworks now.

There's definitely more - Sorry, I'm running out of time here...

Betthel

Answer 2

Struts Actions, in my mind, shouldn't have very much code in them at all. They should just interact directly with the request and response - take some data from a form or a request parameter, hand that info off to the Service Layer, and then put some stuff in a Response object or maybe save some data in the user's session.

I'd recommend staying away from doing inheritance with action classes. It sounds like a good idea at first but I think sooner or later you realize that you're shoe-horning things more than you're actually making the code base robust. Struts has enough base actions as is, if you're creating new ones you've probably got code in the web layer that shouldn't be there.

That is just my personal experience.

Massacre

Answer 3

I've dealt with this type of thing before. A good first step is to insert another base class into the inheritance chain between Action and one of the original monstrous action classes (lets call it ClassA). Especially if you don't have time to do everything at once. Then you can start pulling out pieces of functionality into smaller parallel Action classes (ClassB, ClassC). Anything that's common between the original ClassA and the new refactored classes can be pulled up into the new base class. So the hierarchy now looks like this:

Original Hierarchy:      New Hierarchy:

     Action                   Action
       |                        |
       |                      BaseA
  (old)ClassA                   |
                       +--------+----------+
                       |        |          |
                   ClassB (new)ClassA   ClassC

Olson

Read my answer above for thoughts on having an Action hierarchy. I think in a lot of places that do this they probably have too much business logic in their web layer. – Massacre

Answer 4

1. Go one method at a time
2. Record some test cases you can play back later. Example here (make sure to hit as many paths through the code as you can, i.e. all user gestures on the page that call this action)
3. refactor the method to reduce its complexity by creating smaller methods that do smaller things.
4. Re-run tests as you do this

At this point, you have refactored version of the big huge annoying method. Now you can actually start creating specific actions.

You can use your newly refactored class as a base class, and implement each specific action as a subclass using those refactored small methods.

Once you've done this, you should have a good picture of the logic shared between the classes and can pull-up or push-down those methods as needed.

It's not fun, but if you will be working on the codebase for a while, it will save you time and headaches.

Buffy

Answer 5

Tough problem but typical of early web app development.

First things first you need to start thinking about which logic constitutes business behavior, which logic constitutes "flow" (i.e. what the user sees), and which logic gets the content for what he sees.

You don't have to go down the route of factories and interfaces and all that; retroactive implementation is far less useful... but consolidating business logic and data retrieval logic into delegates of some kind... and leaving the struts actions to determine page flow based on success/failure of that logic.

From there you just have to take a few weeks and grind it out

Crawley

Answer 6

One long method is never good, unless it happens to be a single switch statement where the cases are very short (token parsing or something like that).

You could at least refactor the long method into smaller methods with descriptive names.

If at all possible you could start your method with recognizing what it is it should do by examining the form, and then if/else your way to the various options. No nested ifs though, those tend to make code unreadable. Just

enum Operation {
  ADD, DELETE;
}

...

Operation operation = determineOperation(form);
if (operation == Operation.DELETE) { 
  doDelete(form); 
} else if (operation == Operation.ADD) {
  doAdd(form);
}

If you can go that far you have your logic nice and clean and you can do whatever refactoring you want.

The hard part is to get your logic clear, and you can do that in steps. Don't choose a pattern untill you understand exactly what your problem is.

Dimissory

Answer 7

If you're planning to refactor the code you should make sure to write tests for the existing code first so you can be sure you haven't altered the functionality of it once you start refactoring.

Sonorous

I'm going to say pshah. The author states that the methods are monstrously long and have a high cyclomatic complexity. That kind of code is often a nightmare to unit test. – Reider