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

21–30 of 95 posts

Re: Code Smells: Iteration

#21

Post by mikeash »

chillydawg wrote: post #6
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 obvious to anyone with a CS (or similar) degree, but may not be to someone who is learning to code from various tutorials. Having hints like this in your IDE can greatly improve the quality of your code and help you learn.

Exactly, this is pretty clearly aimed at beginner/intermediate programmers who haven't entirely figured out how to decide when to use List versus Set versus Map. It's probably obvious to the typical HN reader, but valuable for the target audience. Might not belong here, although the fact that it's currently at #3 on the front page suggests there's enough interest in it.

Re: Code Smells: Iteration

#22

Post by hedora »

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…

The "smell" here is that the API does O(n) allocations for a read-only check of a database schema?

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

Re: Code Smells: Iteration

#23

Post by dionian »

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…

Many of the syntax highlighting that IDEA does is very obvious once you're alerted. The nice thing is it does hundreds if not thousands of analyses and presents them to you as actionable feedback.

Re: Code Smells: Iteration

#24

Post by dionian »

chillydawg wrote: post #6
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 obvious to anyone with a CS (or similar) degree, but may not be to someone who is learning to code from various tutorials. Having hints like this in your IDE can greatly improve the quality of your code and help you learn.

People with CS degree and decades of real world experience still benefit from automated code analyses. We deal with systems with millions of lines of code...

Re: Code Smells: Iteration

#25

Post by wereHamster »

koolba wrote: post #8
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…

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. Tests help, a good IDE helps (to find location where a symbol is used). But still, it's a fine line to walk.

Re: Code Smells: Iteration

#27

Post by hinkley »

jayd16 wrote: post #17
hinkley wrote: post #13

Earlier quoted context omitted.

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…

Just FYI, they ran this example over some built in reflection APIs. Hard to write code designed to get methods by string name without strings...

Exhibit A:

    The getLoadNames method returns a List of Strings, which we iterate over in order to see if a particular String value is in there.
Even in describing the algorithm we aren't informed it's a list of function names.

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.

Re: Code Smells: Iteration

#28

Post by nemetroid »

jstimpfle wrote: post #18
megawatthours wrote: post #12

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 / fo…

What is the "most restrictive" form? The answer to this question is highly context dependent (for example, programming language / architectural framework) and expensive to give. Remember that everything has a cost. And especially overzealous formalism. 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…

Map-style iteration carries more semantic information than a C-style index based loop. An index based loop might be doing anything with the data being iterated over. When you do collection.map, already at first glance you get an idea about what the code is doing.

Re: Code Smells: Iteration

#29

Post by hedora »

recursive wrote: post #20
hinkley wrote: post #13

Earlier quoted context omitted.

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…

Unix started it.

Even C has types, and the UNIX API's make heavy use of them.

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.

Re: Code Smells: Iteration

#30

Post by Ar-Curunir »

ubertaco wrote: post #3

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.

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.
Post reply on HN