Refactoring into ravioli code
in [OOP]
|
Prev: How should a container class "know" its contained objects?
Next: china wholesale nike jordan sneakers WWW.21cn-shoes.COM discount gucci prada puma
From: chris_a_brooks on 15 Jan 2008 06:38 WikiPedia would seem to disagree but I would see ravioli as an example of bad OO. The following would seem to me to be a (nicely contentious) example of refactoring into ravioli. I have a class that has several match methods, I show two below but there are others. They are central to the classes duty. The code __is__ repetitive, the only change being the test made. allStrings is a list of source strings. public String[] matchNamesThatStartWith (String matchMe) { String [] matches; List<String> workingMatches = new ArrayList<String>(); for (String stringToTest: allStrings) { if (stringToTest.startsWith(matchMe)) workingMatches.add(stringToTest); } matches = new String[workingMatches.size()]; workingMatches.toArray(matches); return matches; } public String[] matchNamesThatEndWith (String matchMe) { String [] matches; List<String> workingMatches = new ArrayList<String>(); for (String stringToTest: allStrings) { if (stringToTest.endsWith(matchMe)) workingMatches.add(stringToTest); } matches = new String[workingMatches.size()]; workingMatches.toArray(matches); return matches; } So this could be refactored. Create a root abstract class that iterates the list calling a test method implemented in specific subclass. You'd probably add an interface. So to solve our example of two test methods above we would add four very simple classes. But ! IMHO the code is now less self documenting. The code as was just needs a glance to understand what it's doing. The methods are short. The revised version would need more time to open and drill down into the two classes (and interface) that solve the task for a given method. I agree once you're there the code is you see is very simple, you can add new tests more simply, but you have had to follow a thread of execution to understand it. This is the essence to me of the problem that ravioli causes. WriteItForReading is another mantra I'd like to see in the WikiWikiWeb. Readability over overly merciless refactoring every time I'd say. __Anyone__ agree? Chris
From: Daniel T. on 15 Jan 2008 08:22 chris_a_brooks(a)hotmail.co.uk wrote: > WikiPedia would seem to disagree but I would see ravioli as an example > of bad OO. The following would seem to me to be a (nicely contentious) > example of refactoring into ravioli. > > I have a class that has several match methods, I show two below but > there are others. They are central to the classes duty. The code > __is__ repetitive, the only change being the test made. allStrings is > a list of source strings. > > public String[] matchNamesThatStartWith (String matchMe) { > > String [] matches; > List<String> workingMatches = new ArrayList<String>(); > > for (String stringToTest: allStrings) { > if (stringToTest.startsWith(matchMe)) > workingMatches.add(stringToTest); > } > > matches = new String[workingMatches.size()]; > workingMatches.toArray(matches); > return matches; > } > > public String[] matchNamesThatEndWith (String matchMe) { > > String [] matches; > List<String> workingMatches = new ArrayList<String>(); > > for (String stringToTest: allStrings) { > if (stringToTest.endsWith(matchMe)) > workingMatches.add(stringToTest); > } > > matches = new String[workingMatches.size()]; > workingMatches.toArray(matches); > return matches; > } What language is that? C#? > So this could be refactored. Create a root abstract class that > iterates the list calling a test method implemented in specific > subclass. You'd probably add an interface. So to solve our example of > two test methods above we would add four very simple classes. That's exactly what is done in SmallTalk, except they narrowed down to two simple classes: Collection and Block. see below. > But ! > > IMHO the code is now less self documenting. The code as was just needs > a glance to understand what it's doing. The methods are short. The > revised version would need more time to open and drill down into the > two classes (and interface) that solve the task for a given method. I > agree once you're there the code is you see is very simple, you can > add new tests more simply, but you have had to follow a thread of > execution to understand it. This is the essence to me of the problem > that ravioli causes. More importantly, once you understand such code, *and apply it throughout your system*, the entire system becomes easer to understand and simpler to deal with. > WriteItForReading is another mantra I'd like to see in the > WikiWikiWeb. Readability over overly merciless refactoring every time > I'd say. Exactly. Iterating through a collection and gathering all the items that meet some test, is a pretty common thing to do. Not having a common abstraction to do that is less readable IMO. > __Anyone__ agree? Apparently Martin Fowler (the writer of the book "Refactoring") agrees with you. "With loops, extract the loop and the code within the loop into its own method."(77) However, it is interesting to see how this is handled in some other languages... SmallTalk (Squeak): matchNamesThatStartWith: matchMe ^self allStrings select: [:each| each startsWith: matchMe]. matchNamesThatEndWith: matchMe ^self allStrings select: [:each| each endsWith: matchMe]. C++ template < typename OutIt > void matchNamesThatStartWith( String matchMe, OutIt matches ) { remove_copy_if( allStrings.begin(), allStrings.end(), matches, !bind( &String::startsWith, _1, matchMe ) ); } template < typename OutIt > void matchNamesThatEndWith( String matchMe, OutIt matches ) { remove_copy_if( allStrings.begin(), allStrings.end(), matches, !bind( &String::endsWith, _1, matchMe ) ); } See what happens when the system has at least made an attempt to create a "select items that meet a criteria" abstraction? You end up with the ability to select members of a container with one line.
From: chris_a_brooks on 15 Jan 2008 08:54 On 15 Jan, 13:22, "Daniel T." <danie...(a)earthlink.net> wrote: > > What language is that? C#? Java. Must admit I hate all the new generics stuff... > SmallTalk (Squeak): > matchNamesThatStartWith: matchMe > > ^self allStrings select: [:each| each startsWith: matchMe]. Well of course But you have created the code __here__, in the top level matchNamesThatStartWith method. You have not created more classes (and hence ravioli - my original point) to solve this issue. You've just used the fact Smalltalk provides this kind of language construct to help you. The C++ example I didn't understand I'm afraid (having only played with the language 15 years ago) No disrespect intended but were you suggesting that >> remove_copy_if( allStrings.begin(), allStrings.end(), matches, !bind( &String::endsWith, _1, matchMe ) ); was self documenting? >.You end up with the ability to select members of a container with one line.. I'm sure you'd agree the ability to put the all the code on one line does not necessarily make for a readable line of code. It's all about writing code that can be __read__ (assuming familarity with the syntax) not which has to be __translated__. Your c++ must be translated to be understood. The Smalltalk example was fine obviously. Thanks for the comments. Chris
From: Daniel T. on 15 Jan 2008 10:45 chris_a_brooks(a)hotmail.co.uk wrote: > "Daniel T." <danie...(a)earthlink.net> wrote: > > > SmallTalk (Squeak): > > matchNamesThatStartWith: matchMe > > > > � �^self allStrings select: [:each| each startsWith: matchMe]. > > Well of course > > But you have created the code __here__, in the top level > matchNamesThatStartWith method. You have not created more classes > (and hence ravioli - my original point) to solve this issue. You've > just used the fact Smalltalk provides this kind of language > construct to help you. You misunderstand the code. I didn't need to create more classes because they already existed in the library. I didn't use special constructs that SmallTalk provides, I used the well factored library that is provided. You don't mention what class "allStrings" is an object of, but if it had a "select" method, would you refuse to used it because it's too "ravioli" like? I think not. > The Smalltalk example was fine obviously. Then you might find it interesting how 'select:' is implemented, it isn't a language construct... "In class Collection" select: aBlock | newCollection | newCollection := self species new. self do: [:each | (aBlock value: each) ifTrue: [newCollection add: each]]. ^newCollection 'do:' is abstract (called a "pure virtual function" in C++,) each Collection leaf class implements it as it needs. For example OrderedCollection implements it this way: do: aBlock | index | index := firstIndex. [index <= lastIndex] whileTrue: [aBlock value: (array at: index). index := index + 1] I didn't have to use 'select:' in my original code. If I hadn't, it would have looked something like: matchNamesThatStartWith: matchMe |workingMatches| workingMatches := OrderedCollection new. self allStrings do: [:stringToTest | stringToTest startsWith: matchMe ifTrue: [workingMatches add: stringToTest]] ^workingMatches See how it's starting to look very much like the code you originally posted? (I deliberatly used the same variable names and formatting you used to help make that implression.) Take that one step further. If I knew that allStrings was an OrderedCollection object I could write... |workingMatches index| workingMatches := OrderedCollection new. index := allStrings firstIndex. [index <= allStrings lastIndex] whileTrue: [allStrings array at: index startsWith: matchMe ifTrue: [workingMatches add: allStrings array at: index] index := index + 1] ^workingMatches. Now it's looking downright C like! The point I'm trying to make here is that "allStrings select: [:stringToTest | stringToTest startsWith: watchMe]." is easy to read and expressive precisely because the container classes have been factored in the way you suggest is inappropriate. (Warning: I am not an expert at SmallTalk, the code above may have several syntax errors.)
From: H. S. Lahman on 16 Jan 2008 11:29
Responding to Chris_a_brooks... > I have a class that has several match methods, I show two below but > there are others. They are central to the classes duty. The code > __is__ repetitive, the only change being the test made. allStrings is > a list of source strings. > > public String[] matchNamesThatStartWith (String matchMe) { > > String [] matches; > List<String> workingMatches = new ArrayList<String>(); > > for (String stringToTest: allStrings) { > if (stringToTest.startsWith(matchMe)) > workingMatches.add(stringToTest); > } > > matches = new String[workingMatches.size()]; > workingMatches.toArray(matches); > return matches; > } > > public String[] matchNamesThatEndWith (String matchMe) { > > String [] matches; > List<String> workingMatches = new ArrayList<String>(); > > for (String stringToTest: allStrings) { > if (stringToTest.endsWith(matchMe)) > workingMatches.add(stringToTest); > } > > matches = new String[workingMatches.size()]; > workingMatches.toArray(matches); > return matches; > } > > So this could be refactored. Create a root abstract class that > iterates the list calling a test method implemented in specific > subclass. You'd probably add an interface. So to solve our example of > two test methods above we would add four very simple classes. As an alternative you might consider the Strategy pattern. The problem here is redundancy because the only difference between these methods is the comparison. If you delegate that to a peer object that is subclassed for the different comparison contexts and access it polymorphically, then you only need one method for the iteration and someone to instantiate the relationship properly for the matching context. > > But ! > > IMHO the code is now less self documenting. The code as was just needs > a glance to understand what it's doing. The methods are short. The > revised version would need more time to open and drill down into the > two classes (and interface) that solve the task for a given method. I > agree once you're there the code is you see is very simple, you can > add new tests more simply, but you have had to follow a thread of > execution to understand it. This is the essence to me of the problem > that ravioli causes. I agree with Daniel T. that I don't see a problem with the abstract class. To me delegating the compare responsibility makes the code more readable -- once one is used to the OO paradigm. In effect it is a design idiom. One can argue that a major aspect of the OO paradigm is to capture business rules and policies in static structure rather than executable statements. Subclassing and polymorphic dispatch are important OO tools for doing that (e.g., the context rule is captured by instantiating a static relationship to an object of the right class). Whether the delegation is done with Strategy or subclass this class, one is presenting different views of the solution at different levels of abstraction. If all one cares about in a particular reading context is that one extracts strings from allStrings (e.g., one is, say, just interested in the source of the strings and how they are stored), the multiple methods for doing that and the matching are details that just distract the reader. OTOH, if one wants to ensure that, say, all possible comparisons are provided, one also has exactly one place to look for that but without the distraction of the source iteration and storage mechanisms. IOW, one is using separation of concerns and encapsulation to manage complexity via divide-and-conquer. History has demonstrated that is a good approach on a par with things like the scientific method. So... > > WriteItForReading is another mantra I'd like to see in the > WikiWikiWeb. Readability over overly merciless refactoring every time > I'd say. Remember that much of software development is about compromises among conflicting goals. Sometimes readability needs to take a back seat to other issues. When one is focused on maintainable software, redundancy will normally trump readability because it greatly increases the probability of inserting defects when changes are made. [Also note that using a method for a compare can be a serious performance issue in some circumstances (by the '90s using C's qsort was grounds for dismissal in some C shops). There can be times where the compare method must be inlined directly in individual iteration methods and that will add substantially to redundancy. But customer performance requirements trump developer maintainability requirements.] However, I come back to Daniel T.'s point that essentially this sort of refactoring is an OO idiom. More to the point, the OO paradigm strongly encourages it. As such it really should not detract significantly from the overall readability and, more important, the overall maintainability of the application. -- There is nothing wrong with me that could not be cured by a capful of Drano. H. S. Lahman hsl(a)pathfindermda.com Pathfinder Solutions http://www.pathfindermda.com blog: http://pathfinderpeople.blogs.com/hslahman "Model-Based Translation: The Next Step in Agile Development". Email info(a)pathfindermda.com for your copy. Pathfinder is hiring: http://www.pathfindermda.com/about_us/careers_pos3.php. (888)OOA-PATH |