CodeSOD: A Repetition of Repetition to Repeat

Inheritance is often described as a tool for code reuse, and those discussions inevitably wander down an alley parsing out the nature of has a and is a relationships, when to favor composition, and how inheritance trees get fragile.

When Megan D inherited a Java application which had been largely untouched since 2006, she expected it to be full of all kinds of ugly problems. But they couldn't be that ugly- the software was used and well liked by the end users. Of course, it was hard to tell if they actually liked it, or had just been with it so long they'd adjusted to its flaws, because the whole workflow was extremely fragile and frequently failed.

One feature in the large application was a reporting subsystem which could display line graphs. The three classes we'll look at form a nice inheritance hierarchy. There is the LineGraphTableModel, and then specifically the XLineGraphTableModel and the YLineGraphTableModel.

These classes exist to let the user configure the way the graph renders. The user interface displays the options that can be set for each axis in a JTable view. Each data series on the graph gets a row in the table, each setting you can control on the data series gets a column.

The first thing we'll highlight is that LineGraphTableModel contains a enum called TableColumns, which are the columns on our table.

/** * The table columns used on the SeriesSettings JTable. * * @author Coop */public static enum TableColumns {// DataItem, Label, Color, Points, IndAxis;Label, Color;private TableColumns() {}/** * Retrieves a list of the table column names. * * @return String[] The table column names. */public String[] listColumnNames() {TableColumns[] aValues = TableColumns.values();String[] aNamesList = new String[aValues.length];for (int aLoopIndex = 0; aLoopIndex < aValues.length; aLoopIndex++) {aNamesList[aLoopIndex] = aValues[aLoopIndex].name();}return aNamesList;}}// end enum

None of this is... wrong per se. As Megan writes: "It's just that everything about it is so completely pointless, useless, over-complicated and verbose for how little it does." The method listColumnNames is a good example: while mapping enums to strings might be a useful function, this isn't actually invoked anywhere. It was included as a "just in case" thing.

That's just weird, sure, but not really a WTF.

Now the LineGraphTableModel inherits from the Java AbstractTableModel, which has a few hooks you need to implement to actually work as a table model object. The default implementation covers a lot of cases, but you may need to override a few. For example, if you want to control what column names display on the table:

/** * Implementation for AbstractTableModel to return the name of a indicated * column. * * @param aColIndex * The index number of the column. * @return String The name of the column. */@Overridepublic String getColumnName(int aColIndex) {return TableColumns.values()[aColIndex].name();}

That's fine and pretty normal. But what about actually interacting with the values in the table? You need something for that. Let's see how that gets implemented.

/** * Implementation for AbstractTableModel to set the value in the JTable at the * cell indicated. * * @param aRowIndex * The row index of the cell. * @param aColIndex * The column index of the cell. */@Overrideabstract public void setValueAt(Object value, int aRow, int aCol);

Oh... that's weird. This seems like the kind of thing you'd want to implement in your base class. How different do the implementations of XLineGraphTableModel and YLineGraphTableModel need to be?

Well, let's take a diff of the two files, before we post them in their entirety:

6,7c6,8< /**< * The Class XLineGraphTableModel.---> > /**> * The Class YLineGraphTableModel.9c10< public class XLineGraphTableModel extends LineGraphTableModel {---> public class YLineGraphTableModel extends LineGraphTableModel {12c13< log = LogFactory.getLog(XLineGraphTableModel.class.getCanonicalName());---> log = LogFactory.getLog(YLineGraphTableModel.class.getCanonicalName());26c27< ArrayList<SeriesSettings> alSS = lgm.getXDataSeriesSettings();---> ArrayList<SeriesSettings> alSS = lgm.getYDataSeriesSettings();30c31< }---> }36c37< SeriesSettings.setXAxisColor(value);---> SeriesSettings.setYAxisColor(value);51c52< ---> 55,57c56,58< ArrayList<SeriesSettings> aXDataSeriesList = lgm.getXDataSeriesSettings();< if (aXDataSeriesList.size() < aRow) {< log.debug("Invalid row: "+aRow);---> ArrayList<SeriesSettings> aYDataSeriesList = lgm.getYDataSeriesSettings();> if (aYDataSeriesList.size() < aRow) {> log.debug("Invalid row: " + aRow);60c61< SeriesSettings aDataSeries = aXDataSeriesList.get(aRow);---> SeriesSettings aDataSeries = aYDataSeriesList.get(aRow);

They're the same implementation with the name of fields changed. And here's where we have our true WTF. These two classes are basically copy/paste versions of each other. Except they weren't exactly copy/paste versions of each other, because I cheated a smidge on the diff I posted. They didn't originally have identical whitespace- each file had a few extra line breaks in different places, implying someone was manually copy/pasting (or worse, retyping) the same code into two files.

Now, again, this is just the reporting piece of the product. There is a data parsing piece, there's also an admin piece. Megan has this to say about the entire suite of tools:

The parser is OK, I guess. It's hard-coded and brittle, but the files it reads are generated by an equally hard-coded and brittle C++ program, so it hasn't needed much attention. The admin tool is... fine, though it doesn't do much. Especially now that I took out the buttons that completely wiped out the database without so much as a warning. (Yes. Really. Three buttons that deleted tables entirely just to create them again. Pressing those three buttons in order was part of the "installation" process, but they were there after installation too - between "Test Database Connection" and "Export Processing Queue".)

In any case, here's XLineGraphTableModel in its entirety. Converting it to YLineGraphTableModel can be left as an exercise for the reader.

package com.company.project.app.TOOL_GRAPH.lineGraph;import java.util.ArrayList;import org.apache.commons.logging.LogFactory;/** * The Class XLineGraphTableModel. */public class XLineGraphTableModel extends LineGraphTableModel {/** The Constant log. */protected static final org.apache.commons.logging.Log log = LogFactory.getLog(XLineGraphTableModel.class.getCanonicalName());/** The Constant serialVersionUID. */private static final long serialVersionUID = -7202378534233431981L;/* (non-Javadoc) * @see com.company.project.app.TOOL_GRAPH.lineGraph.LineGraphTableModel#setValueAt(java.lang.Object, int, int) */@Overridepublic void setValueAt(Object value, int aRow, int aCol) {if (lgm == null){return;}ArrayList<SeriesSettings> alSS = lgm.getXDataSeriesSettings();if (alSS.size() < aRow){return;}SeriesSettings aDataSeries = alSS.get(aRow);TableColumns aTableColumn = TableColumns.values()[aCol];switch (aTableColumn) {case Color:aDataSeries.setColor(value);SeriesSettings.setXAxisColor(value);break;case Label:aDataSeries.setLabel(value);break;}fireTableCellUpdated(aRow, aCol);}/* (non-Javadoc) * @see com.company.project.app.TOOL_GRAPH.lineGraph.LineGraphTableModel#getValueAt(int, int) */@Overridepublic Object getValueAt(int aRow, int aCol) {if (lgm == null) {log.debug("Invalid lgm");return null;}ArrayList<SeriesSettings> aXDataSeriesList = lgm.getXDataSeriesSettings();if (aXDataSeriesList.size() < aRow) {log.debug("Invalid row: "+aRow);return null;}SeriesSettings aDataSeries = aXDataSeriesList.get(aRow);TableColumns aTableColumn = TableColumns.values()[aCol];switch (aTableColumn) {case Color:return aDataSeries.color;case Label:return aDataSeries.label;default:return null;}}}

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!