7 ms·
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 a
by jnty 9y ago
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.
- jrs95 9y agoIt's obvious to some of us, but not all of us. I see things way worse than this on a fairly regular basis.
- jnty 9y agoI guess - but I feel like you'd call it an "error" rather than a "smell". Better to just teach people data structures rather than which constructs they should be afraid of. Approaching it "sideways" like this just leads to people going "I heard that loops can be bad, so I copied and pasted that line 25 times instead".
- jayd16 9y agoIts a smell and not an error because it works perfectly fine and it actually is perfectly fine for now because the n for that linear search is low.
- rootlocus 9y agoSets have a very specific use case: they only contain unique values and (unless using a specific implementation) they don't have any defined order. Switching from arrays to sets just to get rid of an iteration doesn't sound like fixing a smell to me. Also, insertion, iteration and memory usage are less efficient for a set.
- JackFr 9y agoImportantly Sets have different semantics than Lists. Creating a set is an information losing process. A list converted to set cannot be recovered, but a set converted to a list is easily recovered. 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.
- CupOfJava 9y agoNaw man, code smells are anything that when you look at code and you wrinkle your face like you would if a cow farted right in front of it.
- chillydawg 9y agoIt'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.
- mikeash 9y agoExactly, 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.
- deleted 9y ago[deleted]
- dionian 9y agoPeople 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...
- koolba 9y agoThat 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 else: doOtherStuff(GOOD_NAME_FOR_MAGIC_CODE); If that triggers an "ah-ha!" moment for you then you've likely got bigger problems.
- wereHamster 9y agoDecreasing 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.
- chairmanwow 9y agoWouldn't this name change be immediately caught by the compiler?
- yxhuvud 9y agoIt is obvious, but the code examples in the original link scare me.
- hinkley 9y agoIt'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 9y agoAgree. I think we're all Stringly-typed developers. We use strings everywhere. We throw strings across system boundaries at will.
- hinkley 9y agoThe first time I ran into this was on a project that was passing contact information around like String, String, String, int. Back and forth across the system in four or five workflows with extra data passed in here and there. 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)
- seanp2k2 9y agoIf 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.
- hedora 9y agoThe "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).
- dionian 9y agoMany 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.
- lstyls 9y agoAgree. 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 iteration to implement a solution in Java.
- burgerdev 9y agoI think the blog post is not so much about advanced code smell mitigation strategies, but rather about highlighting the features of IntelliJ.