A whole post about getting to the refactoring; what nonsense... Wait... I did that... >.<
First thing you gotta ask yourself when refactoring is, "Do I feel lucky?" Well... Do ya? 'Cause if you're feeling lucky; don't worry about unit tests. If you know what you're doing - We need tests.
before starting any kind of major refactor there needs to be test to ensure we're not breaking functionality.
FyzLog has tests, they're available in the gist. Now that we have passing tests (trust me...) we can begin the refactor.
I'll note here; I've done this refactor before. I'm redoing it as I'm writing this; but I have an idea where I'm going already. It might show in a few places.
Looking at the pattern of the log level methods
| public static void wtf(final String msgFormat, final Object... args) { | |
| if (doPrint) { | |
| println(Log.ASSERT, msgFormat, args); | |
| } else { | |
| log(Log.ASSERT, msgFormat, args); | |
| } | |
| } |
We can see that there are two very distinct behaviors happening here; System.out and android.util.Log.
Since it's the 'odd case' in the logger, I'll extract the println behavior first.
During this refactor; I'll be dropping the comments as I perform refactorings. Comments are a smell as they go bad real quick. Most of these comments were originally put in to explain the purpose of each method - Very explicitly a code smell[1].
We're going to perform an Extract Class[1:1] refactor on the println method. This and all dependent methods will be moved into a separate Logger class.
| package com.quantityandconversion.log; | |
| import android.util.Log; | |
| import java.util.Locale; | |
| public class Logger { | |
| private static final String TAG_PREFIX = "FYZ:"; | |
| private static void println(final int level, final String msgFormat, final Object... args) { | |
| if(msgFormat == null) throw new IllegalArgumentException("FyzLog message can not be null"); | |
| final StackTraceElement frame = getCallingStackTraceElement(); | |
| final String output = | |
| getLevelTag(level) + "@" + getLevelTag(logLevel) + "/ " + | |
| createTag(frame) + " " + | |
| createMessage(frame, String.format(Locale.US, msgFormat, args)); | |
| System.out.println(output); | |
| } | |
| private static String getLevelTag(final int level) { | |
| switch (level) { | |
| case Log.VERBOSE: | |
| return "V"; | |
| case Log.DEBUG: | |
| return "D"; | |
| case Log.INFO: | |
| return "I"; | |
| case Log.WARN: | |
| return "W"; | |
| case Log.ERROR: | |
| return "E"; | |
| case Log.ASSERT: | |
| return "WTF"; | |
| default: | |
| return "?"; | |
| } | |
| } | |
| private static String createTag(final StackTraceElement frame) { | |
| final String fullClassName = frame.getClassName(); | |
| final String className = fullClassName.substring(fullClassName.lastIndexOf('.') + 1); | |
| return TAG_PREFIX + className; | |
| } | |
| private static String createMessage(final StackTraceElement frame, final String msg) { | |
| return String.format(Locale.US, | |
| "[%s] %s : %s", | |
| Thread.currentThread().getName(), | |
| frame.getMethodName(), | |
| msg); | |
| } | |
| private static StackTraceElement getCallingStackTraceElement() { | |
| boolean hitLogger = false; | |
| for (final StackTraceElement ste : Thread.currentThread().getStackTrace()) { | |
| final boolean isLogger = ste.getClassName().startsWith(FyzLog.class.getName()); | |
| hitLogger = hitLogger || isLogger; | |
| if (hitLogger && !isLogger) { | |
| return ste; | |
| } | |
| } | |
| return new StackTraceElement(FyzLog.class.getName(), | |
| "getCallingStackTraceElement", | |
| null, | |
| -1); | |
| } | |
| } |
This doesn't compile yet as the extracted class has an unknown variable on lines 15 and 40. We need to refactor println and log with Add Parameter[1:2] to have access to this.
| private static void println(final int level, final int logLevel, final String msgFormat, final Object... args) { | |
| ... | |
| } | |
| private static void log(final int level, final int logLevel, final String msgFormat, final Object... args) { | |
| ... | |
| } |
We've done a chunk of refactoring; Run the tests. ... They still pass... Yes, DUH - We haven't changed the FyzLog, but tests need to be run after each compiling step.
The println method's visibility was private, we'll need to refactor the visibility so it's available in FyzLog.
The Logger class is a class for just FyzLog to use, so let's restrict it's visibility to package-protected
| /* package */ class Logger { ... } |
Run Tests
Now we can refactor the wtf method to call Logger.println; which will require passing in the added parameter.
| public static void wtf(@NonNull final String msgFormat, final Object... args) { | |
| if (doPrint) { | |
| Logger.println(Log.ASSERT, logLevel, msgFormat, args); | |
| } else { | |
| log(Log.ASSERT, msgFormat, args); | |
| } | |
| } |
Run Tests
Apply this refactor to the rest of the log level methods.
Run Tests
Static methods are a bit of a code smell[1:3]; let's refactor Logger into an instantiated class.
The only change to Logger is removing the static modifier from println
| /* package */ void println(final int level, final int logLevel, final String msgFormat, final Object... args) { | |
| ... | |
| } |
Then we're able to update FyzLog to instantiate
| public static void wtf(@NonNull final String msgFormat, final Object... args) { | |
| if (doPrint) { | |
| new Logger().println(Log.ASSERT, logLevel, msgFormat, args); | |
| } else { | |
| log(Log.ASSERT, msgFormat, args); | |
| } | |
| } |
Run Tests
This refactor is applied to each log method; creating a new object EVERY LOG!!! I'm against premature optimization[1:4], but this isn't premature. We have a stateless object. This is, what should be, a rare case where we need to Limit Instantiation with Singleton[1:5].
| /* package */ class Logger { | |
| /* package */ static final Logger SystemOut = new Logger(); | |
| private Logger(){} | |
| ... | |
| } |
And make FyzLog compile using this refactoring
| public static void wtf(@NonNull final String msgFormat, final Object... args) { | |
| if (doPrint) { | |
| Logger.SystemOut.println(Log.ASSERT, logLevel, msgFormat, args); | |
| } else { | |
| log(Log.ASSERT, msgFormat, args); | |
| } | |
| } |
Run Tests
Looking through FyzLog our IDE (in this case, AndroidStudio) helpfully highlights FyzLog#println as unreferenced. Let's perform some cleanup on FyzLog as finalization of our earlier Logger Extract Class refactor.
Delete FyzLog#println.
Run Tests
This cleanup also exposed FyzLog#getLevelTag as only being called from FyzLog#println and it can be deleted.
Run Tests
Deleting code; such a great feeling.
This is a pretty good break point for the post. We've gone through and performed, basically, an Extract Class to pull out some related functionality for one of the branches in our logging methods.
You can see the state of FyzLog here.
I see that this post is pretty short via word-count; around 500 words (depending on how much I ramble here) and for a refactor - That's fantastic. Each step should be small. Let's keep the posts on this refactor following that "small step" model.
Next up: Extract Class of the Android Log functionality!
Part 1 - [Part 2] - Part 3 - Part 4 - Part 5