• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

Refactor this

 
Greenhorn
Posts: 18
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi, I would like the expert opinion over here on making this piece of code less smelly.



It is working code so while loop exits properly on some condition
It is basically processing a screen line by line and populating objects
based on the information on the current line.The methods on the summaryPageDetails return false if the line does not match the regular expression

[EJFH: Added CODE tags to preserve formatting ]
[ November 29, 2005: Message edited by: Ernest Friedman-Hill ]
 
Greenhorn
Posts: 21
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
If all the checks to see if a match is found are performed on the summaryPageDetails object, then I'd move the entire contents of the loop into a method on that class.

Then, make each method into a class of its own with a common method call, like public boolean doesRowMatch(...), with whatever arguments are appropriate. Loop through a collection of instances of these classes, calling doesRowMatch() on each instance until one of them matches. You can even make these processor into inner classes so they have access to common data of the caller.

Does that make sense?

Rick
[ November 29, 2005: Message edited by: Rick Goldstein ]
 
santosh kulkarni
Greenhorn
Posts: 18
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Rick, Thanks for the reply
I was kind of thinking along those lines but your explanation made it very clear. It will need a little more work though. SummaryPageDetails is already an inner class!
if the file becomes huge i'll do some more surgery
Santosh
 
Ranch Hand
Posts: 1923
Scala Postgres Database Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I'm seeing the code-smell of repeating the block, and the 'matchFound' being easily eliminated:

Step 1:


use a shortcut-OR:


if all blocks can be combined this way, we don't need the 'continue' any longer:
 
With a little knowledge, a cast iron skillet is non-stick and lasts a lifetime.
reply
    Bookmark Topic Watch Topic
  • New Topic