• magic_lobster_party@fedia.io
    link
    fedilink
    arrow-up
    29
    ·
    2 months ago

    Looks like it’s JavaScript, but in Java I would prefer to use the Stream API, something like this:

    return availableDrivers.stream()
        .filter(driver -> calculateDistance(rider, driver) < 5)
        .filter(driver -> isPreferredVehicle(rider, driver))
        .filter(driver -> meetsRiderPreferences(rider, driver))
        .findFirst()
        .orElse(null);
    

    Then we have:

    private boolean meetsRiderPreferences(Rider rider, Driver driver) {
        if (driver.rating >= 4.5) {
            if (rider.preferences.includes('Premium Driver')) {
                  return driver.isPremiumDriver;
            } else {
                  return true;
            }
        } else if (driver.rating >= 4.0) {
            return true;
        } else {
            return false;
        }
    }
    

    This increases the separation of concern in a neat way, and it becomes more clear what the for loop does at a glance (get the first driver satisfying a set of conditions). The more complicated logic is isolated in meetsRiderPreferences, which now only returns true or false. Reading the method is more about making a mental map of a truth table.

    It’s also easy to expand the logic (add more filter conditions, sort the drivers based on rating and distance, break out meetsRiderPreferences into smaller methods, etc.).

    Not sure how the equivalent in JavaScript would look like, but this is what I would do in Java.

    • MagicShel@programming.dev
      link
      fedilink
      arrow-up
      10
      ·
      edit-2
      1 month ago

      I try to prefer .findAny() over .findFirst() because it will perform better in some cases (it will have to resolve whether there are other matches and which one is actually first before it can terminate - more relevant for parallel streams I think. findAny short circuits that) but otherwise I like the first. I’d probably go with some sort of composed predicate for the second, to be able to easily add new criteria. But I could be over engineering.

      I mostly just posted because I think not enough people are aware of the reasons to use findAny as a default unless findFirst is needed.

      • magic_lobster_party@fedia.io
        link
        fedilink
        arrow-up
        8
        ·
        2 months ago

        For me I have the habit of doing findFirst because determinism is important where I work. But I agree with you if determinism is not of importance.

        • MagicShel@programming.dev
          link
          fedilink
          arrow-up
          3
          ·
          2 months ago

          I would only note that for the vast majority of my experience these streams can only return up to a single match. Determinism isn’t really preserved by findFirst, either, unless the sort order is set up that way.

          Finding the first Jim Jones in a table is no more reliable that finding any Jim Jones. But finding PersonId 13579 is deterministic whether you findFirst or findAny.

          Perhaps you work in a different domain where your experience is different.

    • Kissaki@programming.dev
      link
      fedilink
      English
      arrow-up
      10
      arrow-down
      1
      ·
      edit-2
      2 months ago

      Using early returns and ternary conditional operator changes

      private boolean meetsRiderPreferences(Rider rider, Driver driver) {
          if (driver.rating >= 4.5) {
              if (rider.preferences.includes('Premium Driver')) {
                    return driver.isPremiumDriver;
              } else {
                    return true;
              }
          } else if (driver.rating >= 4.0) {
              return true;
          } else {
              return false;
          }
      }
      

      to

      private boolean meetsRiderPreferences(Rider rider, Driver driver) {
          if (driver.rating < 4.0) return false;
          if (driver.rating < 4.5) return true;
      
          return rider.preferences.includes('Premium Driver') ? driver.isPremiumDriver : true;
      }
      

      dunno if java has them, but in C# switch expressions could put more of a case focus on the cases

      private boolean meetsRiderPreferences(Rider rider, Driver driver) {
          return driver.rating switch {
              < 4.0 => false,
              < 4.5 => true,
              _      => rider.preferences.includes('Premium Driver') ? driver.isPremiumDriver : true,
          };
      }
      

      or with a body expression

      private boolean meetsRiderPreferences(Rider rider, Driver driver) => driver.rating switch {
          < 4.0 => false,
          < 4.5 => true,
          _      => rider.preferences.includes('Premium Driver') ? driver.isPremiumDriver : true,
      };
      

      The conditional has a true result so it can be converted to a simple bool condition as well.

      private boolean meetsRiderPreferences(Rider rider, Driver driver) => driver.rating switch {
          < 4.0 => false,
          < 4.5 => true,
          _      => !rider.preferences.includes('Premium Driver') || driver.isPremiumDriver,
      };
      
  • arendjr@programming.dev
    link
    fedilink
    arrow-up
    20
    ·
    2 months ago

    While I can get behind most of the advice here, I don’t actually like the conditions array. The reason being that each condition function now needs additional conditions to make sure it doesn’t overlap with the other condition functions. This was much more elegantly handled by the else clauses, since adding another condition to the array has now become a puzzle to verify the conditions remain non-overlapping.

    • BrianTheeBiscuiteer@lemmy.world
      link
      fedilink
      arrow-up
      4
      ·
      2 months ago

      To each their own. Some won’t like the repeating code and some won’t like the distributed logic (i.e. you have to read every if and if-else statement to know when the else takes effect). I think the use of booleans like isDriverClose makes the repeated logic less messy and reduces inefficiency (if the compiler didn’t optimize for you).

    • wewbull@feddit.uk
      link
      fedilink
      English
      arrow-up
      16
      arrow-down
      1
      ·
      2 months ago

      It doesn’t hide. It makes them happen first and, here’s the important bit, closes their scope quickly.

      • Zexks@lemmy.world
        link
        fedilink
        arrow-up
        4
        ·
        2 months ago

        The scope is irrelevant it’s a single function class as presented. It was a single method that they broke out “to hide the ifs”. Then they just used compiler specialties to remove the word ‘if’ from the assignments. The comparisons are still there and depending on the execution path those constants may not be so constant during runtime.

      • Zexks@lemmy.world
        link
        fedilink
        arrow-up
        2
        arrow-down
        1
        ·
        2 months ago

        They’re still ifs. They’ve just been lambda’d and assigned to constants.

        • BrianTheeBiscuiteer@lemmy.world
          link
          fedilink
          arrow-up
          4
          arrow-down
          1
          ·
          1 month ago

          branching ≠ if ≠ conditional

          They’re all related but can’t just be used interchangeably. “if” is a reserved keyword to indicate a specific syntax is expected. It’s not the semantics the author was trying to change, it’s the syntax, and the overall point is that you aren’t always required to use the specific “if” syntax to write code just like you’re not required to use “while” to achieve looping.

          • Zexks@lemmy.world
            link
            fedilink
            arrow-up
            1
            arrow-down
            1
            ·
            1 month ago

            If you decompile that code you won’t get lambdas. You get ifs. Because that is how the hardware is build. Ifs/ands/Ors that is what computing is built on. Everything else is flavor.

  • sebsch@discuss.tchncs.de
    link
    fedilink
    arrow-up
    15
    ·
    edit-2
    2 months ago

    Sometimes is it worth to rethink the problem. Especially when your condition is based on set-members. Using quantor logic often simplifies the condition :

    return 
        any(x for x in X if x==condition_a) 
        or all(y for y in Y if y==condition_b) 
        and all(x for x in X if x==condition_c)
    

    I am not sure if JS has something similar, but this often helps by a lot

    • Lupec@lemm.ee
      link
      fedilink
      arrow-up
      4
      ·
      2 months ago

      I am not sure if JS has something similar, but this often helps by a lot

      It does, the some/every array methods would achieve the same results. I use them quite often myself!

  • ArbitraryValue@sh.itjust.works
    link
    fedilink
    English
    arrow-up
    9
    ·
    edit-2
    2 months ago

    My issue with this is that it works well with sample code but not as well with real-world situations where maintaining a state is important. What if rider.preferences was expensive to calculate?

    Note that this code will ignore a rider’s preferences if it finds a lower-rated driver before a higher-rated driver.

    With that said, I often work on applications where even small improvements in performance are valuable, and that is far from universal in software development. (Generally developer time is much more expensive than CPU time.) I use C++ so I can read this like pseudocode but I’m not familiar with language features that might address my concerns.

  • hector@sh.itjust.works
    link
    fedilink
    arrow-up
    9
    ·
    edit-2
    2 months ago

    EDIT: read the article turns out it’s super useful… It gives insight into decision table which is a pattern I did not know about until recently…

    Is this really a recurring design pattern for y’all?

    I mean, you can just use a switch. anyways I’ll read the article and see ;)

    • Carighan Maconar@lemmy.world
      link
      fedilink
      arrow-up
      11
      arrow-down
      2
      ·
      2 months ago

      Decision tables are nice. They hide the important part of the logic away out of view of another programmer trying to figure out a bug in the code.

      Very helpful! You take longer to find and fix bugs, and potentially miss a few extra ones because of stuff like this. Increased tech debt. Highly recommended! 👍

      • hector@sh.itjust.works
        link
        fedilink
        arrow-up
        2
        ·
        2 months ago

        I mean, you can just right click “Definition” in VSCode and see how it works… It’s not that inconvenient.

        It’s easy to read, write and refactor so I don’t really see what you mean.

        • BehindTheBarrier@programming.dev
          link
          fedilink
          arrow-up
          6
          ·
          edit-2
          2 months ago

          What’s fun is determining which function in that list of functions actually is the one where the bug happens and where. I don’t know about other langauges, but it’s quite inconvenient to debug one-linres since they are tougher to step through. Not hard, but certainly more bothersome.

          I’m also not a huge fan of un-named functions so their functionality/conditions aren’t clear from the naming, it’s largely okay here since the conditional list is fairly simple and it uses only AND comparisons. They quickly become mentally troublesome when you have OR mixed in along with the changing booleans depending on which condition in the list you are looking at.

          At the end of the day though, unit tests should make sure the right driver is returned for the right conditions. That way, you know it works, and the solution is resistant to refactor mishaps.

    • JonC@programming.dev
      link
      fedilink
      English
      arrow-up
      4
      ·
      2 months ago

      Also take a look at the Specification Pattern for something similar.

      That’s something I would only use if the logic becomes very complex, but it can help break things down nicely in those cases.

    • einkorn@feddit.org
      link
      fedilink
      arrow-up
      3
      arrow-down
      6
      ·
      2 months ago

      I can’t find it right now, but there is some explanation in “Clean Code” why switches shouldn’t be used all over the place.

      • magic_lobster_party@fedia.io
        link
        fedilink
        arrow-up
        3
        ·
        1 month ago

        In case you’re wondering about the down votes, many think Clean Code is not a good book. It got a few good advice, but it also got bad advice disguised as good advice.

        I don’t think switch statements should always be avoided. There are cases where polymorphism makes things more difficult to maintain. Saying polymorphism should be used over switch statements is not a good advice.

        Here’s an article going into more detail why we should stop recommending Clean Code: https://qntm.org/clean

      • djnattyp@lemmy.world
        link
        fedilink
        arrow-up
        4
        arrow-down
        1
        ·
        2 months ago

        Google for “replace conditional with polymorphism”.

        Just checked and it is in “Clean Code” - Chaper 17; Section G23 “Prefer Polymorphism to if/else or switch/case”.

        • FizzyOrange@programming.dev
          link
          fedilink
          arrow-up
          4
          ·
          2 months ago

          This is really terrible advice. Sometimes it’s better to do that, but definitely not in the example from this article.

          If anyone says you should always prefer polymorphism to switches they are a bloody idiot.

          • PushButton@lemmy.world
            link
            fedilink
            arrow-up
            3
            ·
            2 months ago

            I always love watching people falling for Clown-Bob’s advises…

            Let’s go, let’s eat shit on toasts! It’s just a matter of how thin you can spread it to hide the taste…

  • jbrains@sh.itjust.works
    link
    fedilink
    arrow-up
    8
    ·
    edit-2
    1 month ago

    The fact that the loop is doing “find first driver matching these strange criteria” seems most obviously obscured by the pattern of assigning a value, then killing the loop or not. This strikes me as the part that makes the algorithm tedious to test, since it forces us to use a collection to test the intricacies of the inner conditions.

    Once we isolate “find first driver matching condition” from computing the condition for each driver, I consider the rest a question of personal taste. Specification pattern, composition of filters, something like that. Whatever you find easier to follow.

  • Carighan Maconar@lemmy.world
    link
    fedilink
    arrow-up
    10
    arrow-down
    4
    ·
    2 months ago

    I love how this tries to sell making your code strictly worse as something positive.

    Sigh. And it’s still full of ifs.

    • HamsterRage@lemmy.ca
      link
      fedilink
      arrow-up
      14
      ·
      2 months ago

      You might want to think about it a bit more before putting it to work. The comment with the streams example is far, far better.