Buggy auto conversion to string : In which code-smell would it fall and how should avoid one?

Question

Consider this Java code:

int rowIndex=8;
int offset=0;
libFuncCall.populate("B"+rowIndex+offset, data);

Here I mistakenly missed the fact that, resulting argument is a String though I had rowIndex and offset both integers.
The actual code would have been:

libFuncCall.populate("B"+(int)(rowIndex+offset), data);

Until I logged it, before finding:

log.debug("B"+rowIndex+offset); // It was B80 instead of B8

In my case, first input argument was a cell in Excel spreadsheet. It took me days, to find this bug.

In which code smell catagory would it fall and how should I avoid one?

• Martin Maat: It is not a code smell, just a misinterpretation of a single piece of code. A code smell is a bad trend in a collection of code files, typically a software product.I would say this is a mistake on the language designers' part. In an attempt to make things easier and better looking they made it more dangerous.
• Martin Frank: if this method is the only way to access fields, then it is a code smell - since you address a cell in excel by row & column - not by cellID... even if they look very similar (see #3 on my answer). The pure fact, that the OP did loose some hours on finding the issue is prove for itself. (see #1 on my answer, the interface is not simple enough) - on the other hand, if there are more way to populate cells and the OP did use a misleading one, well... then it would be no code smell... i think we don't have enough data yet to know for sure (+1 for that comment)
• MD. Mohiuddin Ahmed: Yup. I wanted to keep things easy and raw (revisit-refactor later). As I was providing quickfix and kept the "using library provided Cell object+look more into library provided getCellValueByCellName()" as technical-dept. Also I was trying to have cell cordinates abstracted away by simple String cell names(e.g. B8) in my library-agnostic wrapper class. It backfired.

Answer 1

violates KISS:

you have to create an Cell ID where you could provide coordinates - coordinates are way simpler

violates principle of least astonishment

surprise: this cell id is an aggregated key

violates implementation reflects design

cell coordinates are used by design - an cell id is not was specified

of course

the proper method signature would have been libFuncCall.populate(String column, int row)

• Doc Brown: I think one should add that in case ´libFuncCall.populate` is a 3rd party lib with a fixed API, a solution would be to create a wrapper for that call which requires coordinates and use that wrapper exclusively.
• Martin Frank: that sounds reasonable @DocBrown!

Answer 2

In addition to @MartinFrank I*d like to add: primitive obsession

While the code smell focuses on types I find it in the way you construct the string.

Whenever I have to combine a literal string with dynamic data I always use String.format() This would change your code to:

libFuncCall.populate(String.format("B%d",rawIndex+offset), data); 

leaving no room for the mistake you made.

• Martin Frank: yes, very very right (+1)
• IMSoP: Using String.format doesn't solve the "primitive obsession", it just works around it - you have to remember to do that each time. Using a specific type encapsulates that logic, so that the easy thing to do is correct: you "fall into the pit of success". So the final code would be libFuncCall.populate(new CellId("B", rawIndex+offset), data);, with the CellId class responsible for calling String.format when you need to interact with a lower layer that expects the identifier as a string.

Answer 3

You're breaking Curly's law. Do only one thing.

Every piece of code you write should do only one thing.

"B"+rawIndex+offset

You want this piece of code to do three things: Add two integers, convert the result of that to a string, concatenate two strings.

It does two of those things, converting to string and concatenating.

You avoid this by being explicit.

int rawIndex=8;
int offset=0;
Integer cellRowNumber = rawIndex + offset;
String cellAddress = "B" + cellRowNumber.toString(); 
libFuncCall.populate(cellAddress , data);

• Peter M: Obvious question: How do we know that "B" is a cell column?
• MD. Mohiuddin Ahmed: @PeterM: The thing is that, I was completely missing the fact that I was in fact dealing with strings rather than integers, thought some java compile mechanism should notify/warn me beforehand. For a moment I thought, "I can only expect this in dynamically-typed python or javascript". Now I know, I could get away with having/using Cell object of/in API.
• Pieter B: @PeterM it should be even better for cellAddress not be a string at all, but a structure representing an actual location: something like: cellAddress.RowNumber, cellAddress.ColumnLetters . If you want to get to the cell later on you have to break it apart later anyway, so best to not join it in a string in the first place.
• Peter M: @PieterB I agree that a structure/class would be a better approach. But I think was being too subtle in my comment. I was trying to imply that at the very least you should define String cellColumn = "B";, and then String cellAddress = cellColumn + cellRowNumber.toString(); or even String cell Address = makeCellAddress(cellColumn, cellRowNumber);

Answer 4

I think the fundamental issue here is primitive obsession, or stringly typed code: your libFuncCall.populate method lists its input as a string, and leaves the caller responsible for formatting that string correctly. Presumably, other methods also need a similar cell reference; and other places in the code generate one.

That leaves a number of responsibilities scattered around the code:

• Gluing together row and column references (currently the responsibility of the caller)
• Validating that the string is a sane reference (presumably libFuncCall.populate would need to throw or propagate an exception if I passed "hello, world" as the argument)
• Splitting the string back up again if needed (and related tasks such as incrementing to get the next cell in the same column, or the same row)

Using a separate row and column parameter (as Martin Frank suggests) would solve the immediate problem, but still leaves these responsibilities scattered: libFuncCall.populate has to recognise that "hello, world" isn't a valid column reference, and know how to combine the two arguments as a string if lower-level functions expect one.

All of these can be encapsulated in a type such as CellReference:

• If you construct an instance from separate row and column parameters, you can't make the mistake shown in the question
• The constructor can immediately validate those, so libFuncCall.populate can trust the value ("make invalid states unrepresentable")
• The CellReference instance can have separate accessors for column and row, or for a combined string, depending on what the ultimate low-level code actually needs
• The signature of libFuncCall.populate immediately makes clear what it needs, rather than needing additional comments describing the expected format of the string argument

So the final code would look like:

int rawIndex=8;
int offset=0;
libFuncCall.populate(new CellReference("B", rawIndex+offset), data);

A typo of new CellReference("B"+rawIndex+offset) would be a compile error (no single-argument constructor); a typo of new CellReference("B"+rawIndex, offset) would be an immediate run-time error ("B0" is not a valid column reference).

• MD. Mohiuddin Ahmed: new CellReference("B", rawIndex+offset). That sounds reasonable rather than specifying coordinates.