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 :-)