
Code Smells: Iteration - ingve
https://blog.jetbrains.com/idea/2017/08/code-smells-iteration/
======
jnty
At the risk of sounding arrogant, isn't this a bit obvious?

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.

~~~
hinkley
It's worse than that. We have met the Enemy and the Enemy is us.

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.

~~~
nickbauman
Agree. I think we're all Stringly-typed developers. We use strings everywhere.
We throw strings across system boundaries at will.

~~~
seanp2k2
If you're just using objects to represent groupings of strings and encapsulate
behavior around converting them back to strings for output at some point, why
not just operate on the strings instead of all the serialization /
deserialization? Maybe enough of the quick stuff I do isn't enough to warrant
more complex data structures, but I get pretty far into a problem with just
dicts (in Python) and generally get annoyed when something returns back
objects that have no easy str representation without finagling the object into
giving me what I need. I think that this is generally a bigger issue with
libraries and treating object metadata (stuff like time stamps, extra
attributes, etc) as of equal importance as the data it represents; if an
object has one obvious attribute for what the thing is, make that the default
thing you get when you just refer to the thing, instead of making everyone
call some weird method to turn the instance into the representation that you
likely want to get out of it.

~~~
cormacrelf
cat your_comment.txt | fold -w 80 -s | grep that | head -n 1 | sed "s/ have/'d
be/" | sed 's/n/to/' | cut -d's' -f1-2

~~~
msla
If you're going to be snarky, fix the useless use of cat.

------
maweki
The author proposes using data structures that use hashing without even
mentioning the necessity of having a hash function and immutable data for the
hashed objects, because in their case they were basically just using strings.

It isn't always that easy.

------
e12e

      boolean hasName(String
        storedName) {
    
        return getLoadNames().\
           contains(storedName);
        }
    

> That’s it. No more looping, just a simple check.

And how is set.contains implemented?

[https://github.com/openjdk-
mirror/jdk7u-jdk/blob/master/src/...](https://github.com/openjdk-
mirror/jdk7u-jdk/blob/master/src/share/classes/java/util/HashSet.java)

wraps

[https://github.com/openjdk-
mirror/jdk7u-jdk/blob/master/src/...](https://github.com/openjdk-
mirror/jdk7u-jdk/blob/master/src/share/classes/java/util/HashMap.java)

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;
        }
    

TL;DR how did they imagine a general setContains would be implemented without
a loop?

[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...]

~~~
jnordwick
But the loop your pointing out is only over the hash bucket, not the entire
set. Ideally it will only have a single element.

~~~
e12e
Good point :)

------
megawatthours
Something I see often and is a huge code smell to me is not using the most
restrictive form of iteration.

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!

~~~
masklinn
> 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.

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.

~~~
OtterCoder
Even Excalibur can be used to mince garlic, and even a supercomputer can be
used to play Zork.

~~~
masklinn
Yeah I mean my point is "trust but verify", I love restricted iteration
construct, but just because they're being used does not mean they're being
used "properly" unless the language ensures it.

------
ubertaco
I like this kind of article not necessarily because it's revolutionary or
groundbreaking (because it's not), but because it serves as a helpful reminder
sometimes, and can help stick an "observation bias" bug in my brain to notice
more often the sorts of examples it calls out.

~~~
Ar-Curunir
Yup, I already know where sets and maps are better than lists, but this
article now primes my brain to look carefully the next time I write more code.

------
KirinDave
Reading this article, I got a bit bothered.

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.

~~~
needcheapbw
> There are very few cases where a for-loop is the right thing to write.

I'm trying to get my head around that, do you mean any for loop is bad or many
nested loops?

~~~
KirinDave
Indexed loops are just a bad problem waiting to happen. They introduce
arithmetic bugs, are the least efficient way to traverse most any data
structure but a sequential memory segment, maximize the opportunity for error,
and are often harder for compile-time optimizers to work through (since it's
harder to prove what they do in situ).

Abstracting away your iteration is important.

~~~
needcheapbw
Like, for(int i=0; i<10; i++) { // todo } OK well yes there can be bugs using
the index and even C++ back when I was doing it had the Boost library which
had for_each. In Java we got something similar back in Java 5 or 6. Also we
have fancy stream operations now in Java 8 which took me the longest time to
come to appreciate. The only down side is removed compatibility with other
languages like C and C++ and C#, locking in the app to a specific language
more.

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.

~~~
hepta
> The only down side is removed compatibility with other languages like C and
> C++ and C#, locking in the app to a specific language more.

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.

------
codeulike
I really don't like the term Code Smells. Its just seems like it heralds
finicky criticism that ignores wider context and real-world pressures. Outside
of academia, _you can 't release a build without creating a few smells_

------
glibgil
Hashing is indexing and there is no faster and memory efficient index than an
array (not vector) of sorted items. Iteration is the best if you can organize
your data for it

~~~
jdmichal
Not sure what you're trying to get at. Indexing by surrogate index, yes.
Indexing by value? That doesn't help. The best you can do is binary search --
O(log n). Hash-based structures are average amortized constant time -- O(1).
This works because the key is a function of the value, rather than a surrogate
key which is unrelated to the value.

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."

[https://en.wikipedia.org/wiki/Hash_table](https://en.wikipedia.org/wiki/Hash_table)

------
flavio81
To say that iteration should be "considered a code smell" is not a good thing
to do for the programming community.

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.

------
gpvos
The autoplaying code editing animations are really annoying. By the time I've
read the preceding text, it is somewhere in the middle and I don't know what
it's doing.

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).

------
squiguy7
These examples seem to discuss data structure choice what I thought it might
be about. I have encountered two major bugs at two separate companies all
involving iteration and maps.

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.

------
maxscam
Clickbait title, article doesn't seem to back up this hyperbolous claim either

