Thinking in refactoring
Today I have read the article “10 Commandments for Java Developers“. I was confused when I read the third commandment. Author said that the code was more readable in second sample then in first one.
First
1 2 3 4 5 6 7 | if(newStatusCode.equals("SD") && (sellOffDate == null || todayDate.compareTo(sellOffDate)<0 || (lastUsedDate != null && todayDate.compareTo(lastUsedDate)>0)) || (newStatusCode.equals("OBS") && (OBSDate == null || todayDate.compareTo(OBSDate)<0))){ newStatusCode = "NYP"; } |
Second
1 2 3 4 5 6 7 8 | if(newStatusCode.equals("SD") && (sellOffDate == null || todayDate.compareTo(sellOffDate)<0 || (lastUsedDate != null && todayDate.compareTo(lastUsedDate)>0))){ newStatusCode = "NYP"; }else if(newStatusCode.equals("OBS") && (OBSDate == null || todayDate.compareTo(OBSDate)<0)){ newStatusCode = "NYP"; } |
Can you understand the first or the second? Just look and understand? It’s really hard.
Let’s together refactor this code. If you look more closely you can find duplicates. For example,
1 2 | ... sellOffDate == null || todayDate.compareTo(sellOffDate)<0 ... OBSDate == null || todayDate.compareTo(OBSDate)<0) |
There is only one method to simplify the expression - method extraction. Let’s check again… I found another place for method extraction
1 | ... (lastUsedDate != null && todayDate.compareTo(lastUsedDate)>0)) |
After extraction we should get 2 new methods in our class. Next step is extracting variables. It’s very easy and we have next code:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 | class B{ public void testname() { /** * just for fun :) */ Date todayDate = new Date(); String newStatusCode = ""; Date sellOffDate = new Date(); Date lastUsedDate = new Date(); Date OBSDate = new Date(); /** * */ boolean isNewStatusSD = "SD".equals(newStatusCode); boolean isNewStatusOBS = "OBS".equals(newStatusCode); boolean isInPeriod = isBefore(todayDate, sellOffDate) || isAfter(todayDate, lastUsedDate); boolean something1 = isNewStatusSD && isInPeriod; boolean something2 = isNewStatusOBS && isBefore(todayDate, OBSDate); if (something1 || something2) { newStatusCode = "NYP"; } } /** * @param todayDate * @param sellOffDate * @return */ private boolean isBefore(Date todayDate, Date sellOffDate) { return sellOffDate == null || todayDate.compareTo(sellOffDate)<0; } /** * @param todayDate * @param lastUsedDate * @return */ private boolean isAfter(Date todayDate, Date lastUsedDate) { return lastUsedDate != null && todayDate.compareTo(lastUsedDate)>0; } } |
To complete the examlpe we must rename methods and variables. But, of course, I can not be sure my code is working as expected - the business logic may be broken and we have no tests :(, but I think our code is much more easy-to-read. Compare it to 2 sample at the beginning and let me know what you think.
Responses
looking at the samples that need refactoring i think about that dumb analyst (seems like it comes from anal not from analysis) that gave a task to a developer. of course the developer is also weird-minded (look at his code!) but from my experience such pieces of code is a product of situation when business-task is not clearly defined…
P.S.: this is not nevertheless an excuse for a shitty developer. hope he was fired for this piece of pure evil
refactor it the weekly book giveaway
take a look at the post. nice idea.
You are right. nice idea. Who is .Net Developer?
Leave a Reply