Help me refactor this loop

Question

I am working on the redesign of an existing class. In this class about a 400-line while loop that does most of the work. The body of the loop is a minefield of if statements, variable assignments and there is a "continue" in the middle somewhere. The purpose of the loop is hard to understand.

In pseudocode, here's where I'm at the redesign:

/* Some code here to create the objects based on config parameters   */
/* Rather than having if statements scattered through the loop I     */
/* create instances of the appropriate classes.  The constructors     */
/* take a database connection.                                       */

FOR EACH row IN mySourceOfData
  int p = batcher.FindOrCreateBatch( row );
  int s = supplierBatchEntryCreator.CreateOrUpdate( row, p );
  int b = buyerBatchEntryCreator.CreateOrUpdate( row, p );
  mySouceOfData.UpdateAsIncludedInBatch( p, s, b);
NEXT
/* Allow things to complete their last item */
mySupplierBatchEntry.finish();
myBuyerBatchEntry.finish();
myBatcher.finish();

/* Some code here to dispose of things */

RETURN myBatch.listOfBatches();

Inside FindOrCreateBatch() it figures out using some rules if a new batch needs to be created or if an existing one can be used. The different implementations of this interface will have different rules for how it finds them, etc. The return value is the surrogate key (id) from the database of the payment batch that it found or created. This id is required by following processes that take p as a parameter.

This is an improvement over where I started, but I have an uneasy feeling about the class containing this loop.

1. It doesn't seem to a be a domain object, it's more of a "Manager" or "Controller" type class.
2. It seems to be getting inbetween batcher and supplierBatchEntryCreator (and the other classes). At the moment only an int is passed, but if that changes all three classes need to change. This seems like a Law of Dementer violation.

Any suggestions, or is this ok? The actual language is java.

Answer 1

I have a couple of questions to ask you:

• Does it work?
• Is it fast enough?
• Is it readable/maintainable?

If the answer to all three is yes then, beyond that, further changes are really just wasted effort in my opinion. Don't refactor just for the sake of refactoring.

Far too often people change things in anticipation of what might be (your "changing int" for example). I prefer to subscribe to the YAGNI school of thought. The right time to worry about that is when you do it.

And the Law of Demeter is a design guideline, not a rule. In the real world, pragmatism usually beats dogmatism :-)

Answer 2

What is the relationship between each XXXEntryCreator and XXXEntry? I feel like I am missing something, since the "Creators" only return integers.

Beyond that, you took 400 lines of crud down to something that fits on a screen, and has a reasonably visible data flow between steps. Kudos. (I have experienced strong resistance in the past for trying to make such changes -- why do people write N-100/1000 line run-on else-if drivel?)

Answer 3

FindOrCreate and CreateOrUpdate suggest to me that maybe multiple passes would be simpler (and not knowing the rest of the code, I can't know if it would degrade performance, which is a common concern raised when multiple passes are suggested).

If you had one loop to create any missing batches, suppliers, and buyers (or three loops), then this loop could be reduced to

FOR EACH row IN mySourceOfData
  int p = batcher.FindBatch( row );
  int s = supplierBatchEntryCreator.Update( row, p );
  int b = buyerBatchEntryCreator.Update( row, p );
  mySouceOfData.UpdateAsIncludedInBatch( p, s, b);
NEXT

Now I see that the Creator's are updating - is that right? Does splitting the creation and update responsibility into two classes make sense, perhaps?

It's starting to look a little simpler to me. Does it help?