Java RSS Java RSS

Need refactoring opinions

Reply

•
•
Join Date: Jan 2007
Posts: 3
Reputation: mevtho is an unknown quantity at this point 
Solved Threads: 0
mevtho mevtho is offline Offline
Newbie Poster

Need refactoring opinions

 
0
  #1
Feb 24th, 2008
Hi,
I have to refactor the method kingRange under, the goal being code readability :

java Syntax (Toggle Plain Text) 
  1. /*
  2. For information, here are the class fields needed here :
  3. private String setUp;
  4. public final static Collection validPieces =
  5.     Arrays.asList(new String[] { "R", "N", "B", "K", "Q" });
  6. public static final int LEFT = 1;
  7. public static final int RIGHT = 2;
  8.  
  9. */
  10.  
  11. public Range kingRange() {
  12.     Range result;
  13.  
  14.     if ( (setUp.indexOf("R",setUp.indexOf("R")+1) + 1) > 
  15.              (setUp.indexOf("R") + 1) ) {                
  16.         result = new Range((setUp.indexOf("R") + 1) + 1, (setUp.indexOf("R",setUp.indexOf("R")+1) + 1) - 1);
  17.     }
  18.     else {
  19.         int firstEmptyPositionReadingLeftToRight = 0;
  20.         boolean find = false;
  21.         for (int i = 1; i < 8; i++) {
  22.             if (!find && !validPieces.contains(setUp.substring(i-1,i))){
  23.                 firstEmptyPositionReadingLeftToRight = i;
  24.                 find = true;
  25.             }
  26.         }
  27.  
  28.         int firstEmptyPositionReadingRightToLeft = 0;
  29.         find = false;
  30.         for (int i = 8; i > 1; i--) {
  31.             if (!find && !validPieces.contains(setUp.substring(i-1,i))){
  32.                 firstEmptyPositionReadingRightToLeft = i;
  33.                 find = true;
  34.             }
  35.         }
  36.  
  37.         int left = 0;
  38.         int right = 0;
  39.         if ((setUp.indexOf("R") + 1) ==
  40.                firstEmptyPositionReadingLeftToRight - 1) {
  41.             left = firstEmptyPositionReadingLeftToRight;
  42.         } else {
  43.             left = firstEmptyPositionReadingLeftToRight + 1;
  44.         }
  45.         if ((setUp.indexOf("R") + 1) == 
  46.                firstEmptyPositionReadingRightToLeft + 1) {
  47.             right = firstEmptyPositionReadingRightToLeft;
  48.         } else {
  49.             right = firstEmptyPositionReadingRightToLeft - 1;
  50.         }
  51.         result = new Range(left, right);
  52.  
  53.     }
  54.     return result;
  55. }

Here is where I am so far :

java Syntax (Toggle Plain Text) 
  1. public Range kingRange() {        
  2.     if ( (setUp.indexOf("R",setUp.indexOf("R")+1) + 1) > 
  3.          (setUp.indexOf("R") + 1)) {
  4.         return new Range((setUp.indexOf("R") + 1) + 1, (setUp.indexOf("R",setUp.indexOf("R")+1) + 1) - 1);
  5.     }
  6.     return new Range(rangeLeft(), rangeRight());                
  7. }
  8.  
  9. private int rangeRight() {
  10.     int firstEmptyPositionReadingRightToLeft = 
  11.         firstEmptyPositionReadingRightToLeft();
  12.  
  13.     if ((setUp.indexOf("R") + 1) == 
  14.             firstEmptyPositionReadingRightToLeft + 1) {
  15.         return  firstEmptyPositionReadingRightToLeft;
  16.     } 
  17.     return firstEmptyPositionReadingRightToLeft - 1;
  18. }
  19.  
  20. private int rangeLeft() {
  21.     int firstEmptyPositionReadingLeftToRight =
  22.         firstEmptyPositionReadingLeftToRight();
  23.  
  24.     if ((setUp.indexOf("R") + 1) ==
  25.             firstEmptyPositionReadingLeftToRight - 1) {
  26.         return firstEmptyPositionReadingLeftToRight;
  27.     }
  28.     return firstEmptyPositionReadingLeftToRight + 1;
  29. }
  30.  
  31. private int firstEmptyPositionReadingRightToLeft() {
  32.     for (int i = 8; i > 1; i--) {
  33.         if (!validPieces.contains(setUp.substring(i - 1, i))) {
  34.             return i;
  35.         }
  36.     }
  37.     return 0;
  38. }
  39.  
  40. private int firstEmptyPositionReadingLeftToRight() {
  41.     for (int i = 1; i < 8; i++) {
  42.         if (!validPieces.contains(setUp.substring(i - 1, i))) {
  43.             return i;
  44.         }
  45.     }
  46.     return 0;
  47. }

My question about it is : do you think that some improvement can still be done using Fowler refactorings (see http://www.refactoring.com, in particular removing duplicate code, merging methods, ...) without sacrificing the code readability and understandability ?

I don't want a solution, just your opinion, and if you want some clues on how to improve, I want to find things by myself.

Thanks.
Last edited by mevtho; Feb 24th, 2008 at 9:51 pm. Reason: Try to make code more readable
Reply With Quote Quick reply to this message  
•
•
Join Date: May 2006
Posts: 1,826
Reputation: ithelp is a name known to all ithelp is a name known to all ithelp is a name known to all ithelp is a name known to all ithelp is a name known to all ithelp is a name known to all 
Solved Threads: 117
ithelp's Avatar
ithelp ithelp is offline Offline
Posting Virtuoso

Re: Need refactoring opinions

 
0
  #2
Feb 25th, 2008
I think you have done enough refactoring, also put some comments in the code so that it becomes more understandable.
Assignment Help | Web Design Service | Homework Help | 498a
Programming Help | Need Help ? | Job Interview | Top 10 Hosts
Reply With Quote Quick reply to this message  
•
•
Join Date: Jan 2007
Posts: 3
Reputation: mevtho is an unknown quantity at this point 
Solved Threads: 0
mevtho mevtho is offline Offline
Newbie Poster

Re: Need refactoring opinions

 
0
  #3
Feb 25th, 2008
Thanks for your answer, I think that too since to go further it would require to do deep changes, to the detriment of its understandability.

Any other views on it ?
Reply With Quote Quick reply to this message  
•
•
Join Date: May 2007
Posts: 4,439
Reputation: Ezzaral has much to be proud of Ezzaral has much to be proud of Ezzaral has much to be proud of Ezzaral has much to be proud of Ezzaral has much to be proud of Ezzaral has much to be proud of Ezzaral has much to be proud of Ezzaral has much to be proud of Ezzaral has much to be proud of Ezzaral has much to be proud of 
Solved Threads: 510
Moderator
Featured Poster
Ezzaral's Avatar
Ezzaral Ezzaral is offline Offline
Industrious Poster

Re: Need refactoring opinions

 
0
  #4
Feb 25th, 2008
Notice that the only difference in those pairs of methods is the direction of advance, +1 or -1. If you see that much duplication between methods, chances are you can make them one method. Consider how it might work if you defined
Java Syntax (Toggle Plain Text) 
  1. final static int LEFT = -1;
  2. final static int RIGHT = 1;
and passed the direction to search as a parameter to a single method.
Reply With Quote Quick reply to this message  
•
•
Join Date: Nov 2004
Posts: 6,143
Reputation: jwenting is just really nice jwenting is just really nice jwenting is just really nice jwenting is just really nice 
Solved Threads: 212
Team Colleague
jwenting's Avatar
jwenting jwenting is offline Offline
duckman

Re: Need refactoring opinions

 
0
  #5
Feb 25th, 2008
introduce some Enums to replace all those literals.
As people are clearly allowed to attack me but I'm not allowed to defend myself, I no longer post to this site.
Reply With Quote Quick reply to this message  
•
•
Join Date: Jan 2007
Posts: 3
Reputation: mevtho is an unknown quantity at this point 
Solved Threads: 0
mevtho mevtho is offline Offline
Newbie Poster

Re: Need refactoring opinions

 
0
  #6
Feb 26th, 2008
Thank you for your answers, I've taken all you told me into account, and have been able to pursue my refactoring.

Thanks
Reply With Quote Quick reply to this message  
Reply

Quick Reply to Thread
This thread is more than three months old.
Perhaps start a new thread instead?
•
•
•
•
•
•
•
•
Message:
•
•
•
•



Similar Threads
Other Threads in the Java Forum
Thread Tools Search this Thread



2dgraphics account android api apple applet application array arrays automation banking bidirectional binary binarytree birt bluetooth chat chatprogramusingobjects class client code columns component database derby design eclipse encryption error errors expand fractal game givemetehcodez graphics gui guidancer homework html ide if_statement image inheritance integer intellij interface j2me java javadesktopapplications javaprojects jlabel jme jni jpanel jtextfield julia linux list map method methods midlethttpconnection mobile mobiledevelopmentcreatejar monitoring myaggfun netbeans newbie nullpointerexception open-source problem program programming project property recursion reference ria scanner search server set sms sort sourcelabs splash sql sqlite static stop string support swing testautomation threads tree ui unicode validation windows
About Us | Contact Us | Advertise | DaniWeb | Acceptable Use Policy | RSS Feed

©2003 - 2009 DaniWeb® LLC