• (nodebb)

    I'm pretty sure this is Java, not C#. Bonus points for the singular method name getDomainID which actually returns multiple domain ids.

  • Industrial Automation Engineer (unregistered)

    It's nothing a CSV-string can't solve.

  • Naomi (unregistered) in reply to Melissa U

    Agreed; passing an empty array to toArray is a pretty strong tell. (Arrays in Java are pretty much vestigial, and there are backwards compatibility concerns that make them interoperate poorly with proper collections; in modern code, they're used for varargs and pretty much nothing else.) case is a reserved word, but that could be an anonymization issue.

    particularCase.stream().map(i -> i.getDomainId()).collect(toSet()) is the right way to do this.

  • (nodebb)

    Eh, I dunno whats going on here but this is not C#:

    for (ParticularCaseEntity case : particularCase)

    Also there is no base class Set of HashSet. That would make no sense, because it's an IReadOnlySet or IEnumerable (https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.hashset-1?view=net-8.0)

    Considering that it looks to be really bad code, I bet it's either Java, JavaScript or PHP :p

    Addendum 2024-12-11 07:29: A correct C# version would looks like this BTW:

    [MethodImpl(MethodImplOptions.AggressiveInlining)] private static HashSet<string> GetDomainId(ICollection<ParticularCaseEntity> particularCases) => particularCases.Select(particularCase => particularCase.GetDomainId()).ToHashSet();

  • Officer Johnny Holzkopf (unregistered)

    DDD, or D³ - Douglas the Departing Developer - hehehe ehehehe ...

  • netmunky (unregistered)

    reminds me of a perl snippet that i saw scattered all over a codebase in a past job my @ids = grep { ! $seen{$_} ++ } keys %hash;

    this is a convoluted way to get a unique set of keys from a hash. a simpler version would just be my @ids = keys %hash;

  • Rob (unregistered) in reply to Naomi

    And if an array really needs to be returned: particularCase.stream().map(ParticularCaseEntity::getDomainId).distinct().toArray(String[]::new)

  • (nodebb) in reply to netmunky

    Most likely the grep was originally operating on an array and later changed to a hash (making the grep unnecessary as you have shown.)

    With an array, the grep is also unnecessary as one can simply use uniq from List::Util, like so:

    my @unique = uniq @foo;
  • (nodebb)

    Pfffft to all the issues in the code. TRWTF is rehiring dear Douglas (er, well, if they did that in the end rather than just using his CV as kindling for the next company barbeque).

  • (nodebb) in reply to Naomi

    I'm not sure about arrays being vestigial - all collections use them, and they are the fastest option if you need a fixed length modifiable collection.

  • Randal L. Schwartz (github)

    reminds me of a perl snippet that i saw scattered all over a codebase in a past job my @ids = grep { ! $seen{$_} ++ } keys %hash;

    This makes me sad. I see how they got there, but it's still sad.

  • Tim (unregistered)

    Once you've started thinking about functional programming, it's amazing how much OO code is just reinventing map

  • matt (unregistered)

    Another set of mistakes: commas here. "One of the long-tenured developers, Douglas at Patrick's company left, ..." needs one after Douglas. "...having Patrick doing his job, and also picking up the work that Douglas used to do was bad." needs one after do, or the existing one to go. And half spaced hyphens are not dashes. They make you look like a teenager.

  • (nodebb)

    Ha, someone ninja fixed the article - now I wonder if it was a typo or if Remy also doesn't get any infos about the language involved :-)

  • (nodebb)

    @**** ref

    Bonus points for the singular method name getDomainID which actually returns multiple domain ids.

    Yep. And more bonus points for having a parameter of type Collection<T> named singularly: particularcase. If a single instance of a case con contain multiple case entities (which seems likely), then the naming is even worse. The parameter oughth to be particularCaseEntities. They got not only the plurality wrong, but what the collection contains.

    As often said, naming is in fact a hard problem. Or at least there are hard instances of it. Then there are people who give no thought whatever to the names they choose, an/or simply don't understand what they're trying to do.

    Addendum 2024-12-11 12:16: Oops. My first line was supposed to read: @Melissa U ref

    Sorry Melissa.

  • NotYourLocalJavaWizard (unregistered)

    Not to mention that HashSet has had a constructor that takes a Collection since Java 1.2, so even if you needed an array you could just:

    return new HashSet<>(particularCase).toArray(new String[0]);
  • Sauron (unregistered)

    Now that's an epic WTF of a manglement.

  • Code Refactorer (unregistered)

    The command "domainIds.toArray(new String[0])" is very ineffective. It is creating passing an empty array which is not used and garbage-collected, because a bigger array is needed that is created inside the toArray() method. A quicker running is "domainIds.toArray(new String(domainIds.size()))". Here the passed array is used to fill in the data and then returned. I see this type of coding by a lot of inexperienced programmers who never cared to look up why the toArray() method needs such a parameter

  • Rob (unregistered) in reply to Code Refactorer

    The reason why toArray() needs such a parameter is because otherwise the desired type cannot be determined, and the result will be an Object[], not String[]. That's exactly what toArray() without arguments returns. The overloaded toArray could have been toArray(Class<T> elementType) but that too would require reflection to create the array.

    domainIds.toArray(new String[domainIds.size()]) will indeed use the passed array, as long as the collection doesn't grow in the meantime (if shared between multiple threads). I think that since Java 11 the preferred way should be domainIds.toArray(String[]::new), so the collection can allocate its own array based on its current size. It saddens me to say that the current implementation uses 0 as argument (and I've seen no override that does it differently), but that's something that can be improved internally.

  • (nodebb) in reply to Rob

    The truly astounding WTF here is that .toArray() can't determine the type on its own, which was specified when it was constructed:

    Collection<String> domainIds = new ArrayList<String>();

    The default, in this case, should be the underlying type, String[] rather than Object[], imho, had this been better thought out.

  • Rob (unregistered) in reply to gordonfish

    That's because generics didn't exist until Java 5.0, in 2004. Until that time there was just Collection and ArrayList. When generics were introduced Sun (yes, that long ago) had to deal with a lot of existing code that should continue to work without having to recompile it. The solution was type erasure - generics only exist during compile time, and the compiler effectively removes it or replaces it with casts. During runtime we're back at Collection and ArrayList, the generic type String no longer exists. The JVM can therefore not know that we want String[] and not Object[].

    Other languages like C# use reified generics that effectively creates new types on the fly. That means the generic type is built into the new type and therefore remains during runtime.

Leave a comment on “A Set of Mistakes”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article