Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Code Smells: Iteration

blog.jetbrains.com

Post reply on HN
Print view View original

51–60 of 95 posts

Re: Code Smells: Iteration

#51

Post by _ea1k »

wereHamster wrote: post #25
koolba wrote: post #8

Earlier quoted context omitted.

That was my take as well. It's like seeing code with the same constant inlined in multiple places rather than centralized: // Use some inlined string as a magic code value: doStuff("Some Magic Code"); ... // Later on same inlined string: doOtherStuff("Some Magic Code"); versus: // Define it once: const GOOD_NAME_FOR_MAGIC_CODE = "Some Magic Code"; // Use it: doStuff(GOOD_NAME_FOR_MAGIC_CODE); ... // Use it somewhere…

Decreasing code duplication is good of course. But you need to be careful that the two pieces of code are inherently equal and not just by accident. Because once you consolidate the code, for example like you suggest, and somebody comes along and changes `GOOD_NAME_FOR_MAGIC_CODE` to something else, without being aware that it is used in multiple places, he might inadvertently make a change he did not mean to do. Tes…

That is definitely good advice. I've seen some developers get overly obsessed with deduplicating code, and sometimes it really does just overcomplicate things. For example, it is easy to end up making things more brittle by moving a somewhat generic method with very specific pre-conditions and post-conditions into a generic utility library. It is even worse if those pre and post conditions were documented in the original location, but not necessarily copied over to the new shared module.

Re: Code Smells: Iteration

#52

Post by 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

Re: Code Smells: Iteration

#53

Post by flavio81 »

lstyls wrote: post #33
jnty wrote: post #2

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 struct…

Agree. What makes a pattern a code smell is that it is immediately recognizable and is almost never a false positive. Either the immediate implementation is problematic, or something about the larger context of the implementation is problematic. 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 it…

Specially if you require higher performance and there are no tail call optimizations.

Re: Code Smells: Iteration

#54

Post by 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.

Re: Code Smells: Iteration

#55

Post by sbov »

wereHamster wrote: post #25
koolba wrote: post #8

Earlier quoted context omitted.

That was my take as well. It's like seeing code with the same constant inlined in multiple places rather than centralized: // Use some inlined string as a magic code value: doStuff("Some Magic Code"); ... // Later on same inlined string: doOtherStuff("Some Magic Code"); versus: // Define it once: const GOOD_NAME_FOR_MAGIC_CODE = "Some Magic Code"; // Use it: doStuff(GOOD_NAME_FOR_MAGIC_CODE); ... // Use it somewhere…

Decreasing code duplication is good of course. But you need to be careful that the two pieces of code are inherently equal and not just by accident. Because once you consolidate the code, for example like you suggest, and somebody comes along and changes `GOOD_NAME_FOR_MAGIC_CODE` to something else, without being aware that it is used in multiple places, he might inadvertently make a change he did not mean to do. Tes…

The mess of duplicated code is much harder to untangle than de-consolidating code. Especially because subtle differences WILL leak into duplicated code and you won't know if it's on purpose or not.

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.

Re: Code Smells: Iteration

#56

Post by jnordwick »

jimbokun wrote: post #35
jnordwick wrote: post #26

Does anybody here really have to be told this? This is literally CS 101 (your first month of 61B at Cal). Why is this posted here?

So everyone here is required to have a CS degree now? Lots of self taught developers, and people with business or quantitative-but-not-coding backgrounds read Hacker News, I think.

I expect a higher level of content from HN, not basic material. I think other sites are more appropriate for this.

Re: Code Smells: Iteration

#57

Post by lstyls »

flavio81 wrote: post #53
lstyls wrote: post #33

Earlier quoted context omitted.

Agree. What makes a pattern a code smell is that it is immediately recognizable and is almost never a false positive. Either the immediate implementation is problematic, or something about the larger context of the implementation is problematic. 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 it…

Specially if you require higher performance and there are no tail call optimizations.

I'm not making the assertion :). But what I was getting at is that the larger-scope problem from this perspective would be the use of a non-functional language.

Re: Code Smells: Iteration

#58

Post by strictnein »

seattle_spring wrote: post #47
jnordwick wrote: post #26

Does anybody here really have to be told this? This is literally CS 101 (your first month of 61B at Cal). Why is this posted here?

Are you the guy in that same class that points out how "obvious" everything is, in an attempt to prove how much smarter you are to the professor?

Yes. He's in the discussion about building a Hash function in C doing the exact same thing.

Re: Code Smells: Iteration

#59

Post by tXEHOKhd »

hinkley wrote: post #13
jnty wrote: post #2

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 struct…

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 statical…

It's not just Java! I see this all the time in C and C++ as well. Developers seem to be afraid to use types as they were intended. For example, look up any OpenGL question on StackOverflow and you'll see things like what should be an array of 3D vertices with separate x, y, and z components passed as a 1D array of floats.

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

Re: Code Smells: Iteration

#60

Post by 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/...

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 getEntry(Object key) {
        int hash = (key == null) ? 0: \
          hash(key.hashCode());
        for (Entry 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...]

Post reply on HN