• (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)
    Comment held for moderation.
  • (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]);

Leave a comment on “A Set of Mistakes”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article