Code Smells: Iteration
blog.jetbrains.com
blog.jetbrains.com
To me, code smells are more about spotting when someone's been forced to do something a bit weird and convoluted. It's possibly the cleanest solution available given the immediate context, but this is actually a symptom (or "smell") that something is wrong with the wider design of the system.
This article seems more about detecting choices around data structures which are bad in any context and have no real link to the overall system design.
I don't think this is handled well in TFA. Because of some dubious iteration at the call site, the author changes the semantics of getLoadNames(), blithely assuming that duplicates should not be allowed and order is not important.
The author mentions that the function is called at two other places. For all we know the original author was aware of Sets but chose List because it more correctly matched the use of the structure.
// Use some inlined string as a magic code value:
doStuff("Some Magic Code");
...
// Later on same inlined string:
doOtherStuff("Some Magic Code");
// Define it once:
const GOOD_NAME_FOR_MAGIC_CODE = "Some Magic Code";
// Use it:
doStuff(GOOD_NAME_FOR_MAGIC_CODE);
...
// Use it somewhere else:
doOtherStuff(GOOD_NAME_FOR_MAGIC_CODE);
But I meant when you change the definition of that binding (the right-hand side of the equal sign).
If a dev can't be bothered to figure out where different pieces of code use a variable and whether the change is appropriate for them, do you think they will have the foresight to search for the duplicated code when they need to change it everywhere? Any decent dev should do both, and the former is a MUCH easier process than the latter.
Edit: despite the above, I don't completely disagree with your statement.
Or you're just not as experienced. Remember, every day there's someone born who hasn't seen the Flintstones.
Java, as introduced, claimed that nearly everything was an Object. What we got instead was nearly everything is a String.
The Real WTF in this code is that all of the important information is passed around as Strings. The iterator and its source hint at this but she fixes the wrong problem. Ever has this been the way with Java. Despite having a statically typed language nobody turns the data into information and instead your modules just vomit strings at each other. Sometimes in groups of four or more.
Of course they left off Address Line 2 and rather than try to fix this in thirty places as I was told I made an Address object instead.
With very rare exception, a street Address (Address 1) is meaningless without at least the zip code. And god help us if you had two addresses.
Later it took someone a couple hours to fix it for 9 digit zip codes. No further changes unless a new State joins the union (this was for a state court system and they required a domestic mailing address. One of the few times I didn't eventually need to process international addresses)
That's bad.
"We throw strings across system boundaries at will."
That's neither good nor bad, it's just inevitable. The point of not using "strings" is that strings generally do not have the semantics of whatever it is you are really dealing with, because strings are just raw sequences of bytes and are generally too permissive for the specific type. For instance, if you have a URI, that comes with a list of restrictions for validity such as "can't start with a colon". Programming-language strings aren't URLs because they permit starting with invalid scheme declarations, or being empty.
However, when crossing system boundaries, you are exposed in some sense to the raw truth of the hardware, which is "raw sequences of bytes" is the only thing that really exists. When someone sends you something claimed to be a "URI", you really can't blindly count on that, if you want robust code you are going to have to validate that. Even if you work very hard and use higher-level abstractions to try to map things to higher-level types, you still have the problem that the semantics have to be an exact match for this to work; in one program the URI type only permits http and https, in another it permits arbitrary (valid) schemes, and there you go, there's a difference sufficient that you must validate.
To match semantics between two programs sufficiently that you can claim with a straight face that you really, truly aren't throwing "strings" across system boundaries requires incredibly tight coupling between all participating programs.
The getLoadNames method returns a List of Strings, which we iterate over in order to see if a particular String value is in there.
Edit: from the comments in the code it's pulling field names from a Mongo result set, so it's member variables more so than function names. One half of that relationship is still deterministic at runtime. Mongo records can change whenever but your ORM is dealing with Classes and their contents are known at runtime.
Can we at least agree that the chosen example has too many other confounding factors in it?
Awk and sed and grep and friends certainly encouraged strings, but I heard there was an even earlier thing called REPL. From what I've read it is not a coincidence that PERL is an anagram of it.
Just because, say, a function name and a string are represented in memory the same way, that doesn't mean they are the same type; in particular there are many strings which aren't function names, and there are many operations (e.g. append) which make sense for strings but not for function names.
I can think of two things to blame for this:
- Languages which make it complicated, verbose and/or inefficient to introduce new, incompatible names for existing representations (i.e. nominal typing). For example, having to declare a new class in a new file with a private field, a constructor method and a getter method; that's a lot of boilerplate, and those method calls will incur a runtime cost. Haskell's `newtype` is pretty good in comparison, e.g. saying `newtype FunctionName = FN String` lets me use `FunctionName` in type signatures, I'll get a type error if I try to use a `String` as a `FunctionName` or vice versa, and I can construct and destruct a `FunctionName` using `FN` (e.g. `let x = FN "foo"`)
- Developers only coding for the happy path. When writing a test suite, it's important to test that bad outcomes are prevented, as well as just testing known-safe inputs. The same should apply to types: types should be used such that meaningless expressions are ill-typed, as well as just allowing known-good expressions to be well-typed.
Unfortunately these sorts of considerations tend to get derailed by similar-but-unrelated issues, e.g. 'YAGN a `FunctionName` interface because there's only one implementation'.
The end result of all this is Web applications concatenating together a bunch of "strings" which are actually HTML, plaintext, SQL, user input and URL parameters :(
An obvious example is printing: All of a sudden, you need to convert an object of that type to a representation that can be read. Read by what? Humans? Other code? Both to some extent? The representation is dependent upon both the original type and the intended recipient.
(In Lisp, "readable" means "acceptable to the read function, which parses Lisp expressions"; some objects, such as compiled functions, inherently cannot become "readable" in this sense, so they get printed in an unreadable form. Should that be a type error?)
You can even have layers of representation: Length is a type of value, whether it's expressed in inches or centimeters or light-seconds is a representation, and whether it's in ints or floats or strings is another layer to the representation. You can add inches to centimeters with the right conversion, much like you can add numerical values represented ints and strings with the right conversions. The conversions just have to be at the right layer of representation.
Your ideas sound a lot like the original Hungarian notation, BTW: If your language-level types are representations (as in, your type system says int and float, as opposed to semantic notions like pixels-from-edge or alpha-percentage) you can encode the real type information in variable names. People mutilated this to encoding language-level type information in variable names, which is utterly pointless and potentially harmful.
- `serialise`/`deserialise` to produce/consume machine-readable data; must be mutually inverse (which rules out your example of functions).
- `pretty-print` for human consumption; has no inverse.
As a MVP, these could just be wrappers around `read` and `write` (macros would prevent any runtime overhead); the names convey the intended meaning. Later on we could start enforcing some checks, but since Lisp is dynamically typed, we'd have to do runtime tag-checking (e.g. "if type of 'X' is 'function', throw an error"). A static type system/checker would be better.
More generally, each application should be responsible for parsing its input into a domain-specific model, with machine-checked types. A pair of communicating applications may choose to delegate that responsibility to some common library, but they should not assume that their input can be trusted since it's 'coming from that other system'; e.g. they shouldn't pass around a `String` of input as if it were a domain object, and mangle it by pull out particular characters, concatenating things, etc. That `String` should be parsed into its components ASAP, and those should be passed around.
You're right that "apps hungarian" is another possible approach: slightly better than just documentation, but still not manchine-checked. I think the history of apps hungarian degenerating into systems hungarian is another example of this type/representation conflation.
In C++ we see quite a bit of abuse of std::pair<> instead of just making a fucking struct to hold 2 properly named, easy to read pieces of useful data. Nope! Apparently calling all your data members "first" and "second" is better to some people.
I don't know if it's an educational problem, a language issue, or something else, but I see it so frequently that there must be some common underlying cause (or set of causes).
That said... with C++17 destructuring... it's a lot less painful since you can do the equivalent to a std::tie in a single line.
That's surprising, and for me looks completely uncalled for.
EDIT: Just looked into Haskell (because it's easier), and tuples do derive Ord there. Is there some universal convention I'm missing?
Tuples are always compared lexicographically, is the universal convention. Not sure if that's what you might be missing?
It does.
std::sort(pairarray.begin(),pairarray.end()) works about how you'd expect, as long as operator< is defined for the constituent types of the pair.
Why add an API to have a method that checks for containment directly (and hopefully delete the conatiner returning one entirely?)
Also, it probably makes sense to cache the schema data somewhere.
Finally, my gut tells me the check for the existence of this name probably can be moved earlier in execution (like during initialization of the called class), which will cause the system to fail earlier and be easier to debug (so it would be good to check that before touching it).
A functional programming diehard could assert that iteration is a code smell, because recursion is the preferred solution. But there is nothing inherently wrong with using iteration to implement a solution in Java.
It isn't always that easy.
boolean hasName(String
storedName) {
return getLoadNames().\
contains(storedName);
}
And how is set.contains implemented?
https://github.com/openjdk-mirror/jdk7u-jdk/blob/master/src/...
wraps
https://github.com/openjdk-mirror/jdk7u-jdk/blob/master/src/...
public boolean contains(Object o) { return map.containsKey(o); // map is HashMap }
public boolean containsKey(Object key) { return getEntry(key) != null; }
And finally:
final Entry<K,V> getEntry(Object key) {
int hash = (key == null) ? 0: \
hash(key.hashCode());
for (Entry<K,V> e = \
table[indexFor(hash, \
table.length)];
e != null;
e = e.next) {
Object k;
if (e.hash == hash &&
((k = e.key) == key \
|| (key != null \
&& key.equals(k))))
return e;
}
return null;
}
[ed: obviously it makes sense to use a datastructure that more closely match your intent, but the wording here struck me as a bit odd...]
If you see collection.map(...) you know that each iteration is simply a pure function from original element to transformed element, which is an immense help when reading the code.
If you can use only map / filter / takeWhile / join etc to express what you are doing, use those! If not, try and just use reduce / foreach. If not, try and just use for. Only use while if nothing else works!
You'd think so, but I've had colleagues who managed to fuck that up and use map or list comprehension solely for side-effects.
def update(): Try[Unit] = {
parser.parse(...).map(result => updateState(result))
}
def update(): Try[Unit] = {
parser.parse(...) match {
case Success(result) =>
updateState(result)
Success(())
case f@Failure(_) =>
f
}
}
Personally I find simple C index-based for loops consistent and refreshing. And it's typically not a huge deal. If it is, the procedure might be doing too many things at once. (But yes, I use simple "for x in y" style loops in Python or C++ when they make the lion's share of the loops).
Many different types of loops in a single file, over a single datastructure, always remind me of odd syntax highlighting (for example in vim) in so many different colors that it's only a distraction. I don't care to make so many distinctions. I try to focus on the distinctions that we have to make to get a program done.
I agree with the sentiment: people repeat entirely too much code. There are very few cases where a for-loop is the right thing to write. Using generic methods which can be tested and shipped in isolation is basically always better.
But the article seems to imply there are performance concerns in some cases with its talk of using "a data structure". This irked me, because it's not like Set will beat linear time in balanced read-writes and without care the hash tables end up being non-constant as well.
I'm trying to get my head around that, do you mean any for loop is bad or many nested loops?
Abstracting away your iteration is important.
There is on rare occasion, however, sometimes a problem that is solved more clearly or handily using indexing, similar to step indexing in BASIC.
One use case where I've seen stepped indexing useful is in the field of robotics, which uses step characteristics for synchros and servos.
Using a Stream instead of a for loop does very little to make your code incompatible with C. It is already pretty much incompatible (modulo JNI).
What I mean is: if you want something that looks like C just use that, there is no gain in not learning new constructs just because older languages did not have them.
Straight from Wikipedia:
"In many situations, hash tables turn out to be more efficient than search trees or any other table lookup structure. For this reason, they are widely used in many kinds of computer software, particularly for associative arrays, database indexing, caches, and sets."
Present-day CPUs operate on structures by iterating. On a typical high-level programming language, they are at some point done by either (a) iteration or (b) recursion. If the compiler does not do automatic tail call optimization; then (b) recursion can't be made as optimal (in terms of speed and resources) than (a). The latest Java compiler does not perform tail call optimization and probably never will be able to do it. I mention Java because the article is targeted to the Java crowd.
Thus, iteration IS a very important tool for enhancing performance, and real-world (production) systems might have very important performance requirements.
Iteration, in a code, should just be considered... iteration. No kind of smells.
In the past, there was a very important computer scientist called Edsger Dijkstra wrote a paper called "Goto Statement Considered Harmful", GOTO being considered a "code smell": Dijkstra is a very important guy; an unit of measure, the nanoDijkstra, was named in his honor.
However, even afterwards, new programming languages did include a GOTO statement (or some form), because there are special cases when they are needed for higher performance or (believe it or not!) producing more readable code.
Please, can we just agree that animations and videos (and audio) should only play when the reader initiates it? Also, a progress bar would be handy, although it may not always be necessary (for short animal videos and similar).
Both of the examples were written in Go and although language shouldn't matter, iterating over a map is non-deterministic. But each time the author intended for them to construct the returned data structure in a pre-defined order.
We had been suffering from poor caching performance and saw our Cassandra reads spiking as a result. Once we spotted the incorrect cache key construction, our reads to Cassandra dropped significantly allowing for better performance all around.
Perhaps others would have spotted this but it seems innocuous in code review. I know I will be more diligent in the future.