In our batch jobs for data import we had many similar classes for holding the data being imported. Technically they are all different, with different fields, yet conceptually they are all same. I find this conceptual duplication discomforting and have written a single, more generic, class to replace them all.
The refactoring has been inspired by Clojure and its preference of few generic structures such as maps with many functions over the OO way of many case-specific data structures (i.e. classes), as explained for example in this interview of Rich Hickey, starting with “OO can seriously thwart reuse”.
The original code:
https://gist.github.com/jakubholynet/5518100
The refactored code, where all the LogEntry implementations have been replaced by MapLogEntry:
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
public interface LogEntry { | |
// same as before | |
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
// The generic LogEntry implementation | |
// JavaDoc removed for the sake of brevity | |
public class MapLogEntry implements LogEntry { | |
private static final char FIELD_SEPARATOR = '\t'; | |
private Map<String, String> fields = new HashMap<String, String>(); | |
private final String[][] columns; | |
private final StringBuilder tabString = new StringBuilder(); | |
private final Date timestamp; | |
private String key; | |
public MapLogEntry(Date timestamp, String[][] columns) { | |
this.timestamp = checkNotNull(timestamp, "timestamp"); | |
this.columns = checkNotNull(columns, "columns"); | |
} | |
@Override | |
public String toTabDelimitedString() { | |
return tabString.toString(); | |
} | |
public MapLogEntry addInOrder(String column, String value) { | |
checkAndStoreColumnValue(column, value); | |
appendToTabString(value); | |
return this; | |
} | |
public MapLogEntry validated() throws IllegalStateException { | |
if (fields.size() != columns.length) { | |
throw new IllegalStateException("This entry doesn't contain values for all the columns " + | |
"expected (" + columns.length + "). Actual values (" + fields.size() + "): " + toTabDelimitedString()); | |
} | |
return this; | |
} | |
private void checkAndStoreColumnValue(String column, String value) { | |
final int addedColumnIndex = fields.size(); | |
checkElementIndex(addedColumnIndex, columns.length, "Cannot add more values, all " + columns.length + | |
" columns already provided; column being added: " + column); | |
String expectedColumn = columns[addedColumnIndex][0]; | |
if (!column.equals(expectedColumn)) { | |
throw new IllegalArgumentException("Cannot store value for the column '" + | |
column + "', the column expected at the current position " + addedColumnIndex + | |
" is '" + expectedColumn + "'"); | |
} | |
fields.put(column, value); | |
} | |
private void appendToTabString(String value) { | |
if (tabString.length() > 0) { | |
tabString.append(FIELD_SEPARATOR); | |
} | |
tabString.append(valOrNull(replaceFildSeparators(value))); | |
} | |
/** Encode value for outputting into a tab-delimited dump. */ | |
Object valOrNull(Object columnValue) { | |
if (columnValue == null) { | |
return HiveConstants.NULL_MARKER; | |
} | |
return columnValue; | |
} | |
@Override | |
public Date getTimestamp() { | |
return timestamp; | |
} | |
@Override | |
public String getKey() { | |
return key; | |
} | |
public void setKey(String key) { | |
this.key = key; | |
} | |
public MapLogEntry withKey(String key) { | |
setKey(key); | |
return this; | |
} | |
/**Utility method to simplify testing. */ | |
public Map<String, String> asFieldsMap() { | |
return fields; | |
} | |
… | |
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
// Called from a data import job to process individual log lines. | |
// Some time later, the job will call toTabDelimitedString on it. | |
public class PlayerEventsParser … { | |
@Override | |
public LogEntry parse(String logLine) throws LogParseException { | |
… // tokenizing, processing etc. of the data … | |
return new MapLogEntry(timestamp, getColumns()) | |
.addInOrder("userid", userId) | |
.addInOrder("contentid", contentId) | |
.addInOrder("sessionid", sessionId) | |
… | |
.validated(); | |
} | |
} |
Improvements We have replaced about 15 classes with one, made it possible to change the way data are transformed into a tab-separated string at a single place (DRY), and provided a nice, fluent API whose use looks quite similarly at each place. The new MapLogEntry is also much more testing-friendly (it would be a nightmare to modify all the existing classes to support what M.L.E. does).
Objections Somebody might consider a number of primitive POJO classes simpler than one generic class. The one generic class is certainly more complex than a primitive data structure but in total the solution is less complex because there is fewer pieces and the single piece is used in the same way everywhere so the resulting cognitive load is smaller. The former code is more “easy” to understand while the latter is, all in all, more “simple.”
Principles DRY