FML, this codebase is horrible


  • Discourse touched me in a no-no place

    Hey guys, long time no see! (I've been pretty rammed with work lately so haven't been on for a while.)

    I just uncovered this little gem in a project that my team has been asked to work on, and I thought you guys would enjoy laughing at my misfortune for having to maintain such a huge steaming pile of code. This is just a small sample of the WTFery in this codebase.

    Firstly here’s a little bit of context; this is a public facing booking/reservation system. Customers are only allowed to apply one discount to an order, so we look for the largest discount and only apply that one. As a bonus we also need to know which discount type was applied.

    Sounds pretty simple eh? Spoiler alert!!! It's NOT 💩

      decimal discount = 0;
      int maxIndex = -1;
    
      decimal[] vals = new decimal[4];
    
      //are all offers "on"?
      if ((this.LocalResident) && (this.ResortAmbassador) && (SpecialOfferDiscount > 0) && UseSpecialOffer)
      {
          vals[0] = this.SpecialOfferDiscount;
          vals[1] = this.ResortAmbassadorDiscount;
          vals[2] = this.LocalResidentDiscount;
      }
      //how about just the resident and ambassador offers?
      else if ((this.LocalResident) && (this.ResortAmbassador))
      {
          //return the biggest discount...
          vals[0] = 0;
          vals[1] = this.ResortAmbassadorDiscount;
          vals[2] = this.LocalResidentDiscount;
      }
      //or if just the ambassador and special offer?
      else if ((this.ResortAmbassador) && (SpecialOfferDiscount > 0) && UseSpecialOffer)
      {
          //return the biggest discount...  
          vals[0] = this.SpecialOfferDiscount;
          vals[1] = this.ResortAmbassadorDiscount;
          vals[2] = 0;
      }
      //or the ambassador and special offers
      else if ((this.LocalResident) && (SpecialOfferDiscount > 0) && UseSpecialOffer)
      {
          //return the biggest discount...
          vals[0] = this.SpecialOfferDiscount;
          vals[1] = 0;
          vals[2] = this.LocalResidentDiscount;
      }
      //else just an individual offer
      else if (this.LocalResident)
      {
          vals[0] = 0;
          vals[1] = 0;
          vals[2] = this.LocalResidentDiscount;
      }
      else if (this.ResortAmbassador)
      {
          vals[0] = 0;
          vals[1] = this.ResortAmbassadorDiscount;
          vals[2] = 0;
      }
      else if (this.SpecialOfferDiscount > 0 && UseSpecialOffer)
      {
          vals[0] = this.SpecialOfferDiscount;
          vals[1] = 0;
          vals[2] = 0;
      }
      else
      {
          _CurrentDiscount = Discounts.None;
      }
    
      vals[3] = this.DiscountCodeDiscount;
    
      //find the biggest discount...
      for (int i = 0; i != vals.Length; ++i)
      {
          if (vals[i] > discount)
          {
              discount = vals[i];
              maxIndex = i;
          }
      }
                      
      //switch statement to determine which discount has been applied and save as enum value.
      //this can be used externally to flag up any info required by the particular discount applied.
      switch (maxIndex)
      {
          case 0: this._CurrentDiscount = Discounts.SpecialOffer; break;
          case 1: this._CurrentDiscount = Discounts.Amabassador; break;
          case 2: this._CurrentDiscount = Discounts.LocalResident; break;
          case 3: this._CurrentDiscount = Discounts.DiscountCode; break;
      }
    

    WOW, FML! :wtf:

    What if we wanted to add a couple of extra discount types? One more discount type would double the amount of if statements required to 16, and another would make it 32!

    How do we find the max discount? A foreach loop? Who cares about LINQ anyway?

    How do we figure out which discount was selected? Naturally we rely on each discount type being populated in its own specific array index. If someone fucks up and populates the wrong index in one of your many if statements, you're screwed. <sarcasm>It's a good job that it's totally easy and obvious to spot when one of them is wrong.</sarcasm> 🙊

    I find it hard to believe that the guy writing this code didn't ask himself if there was a better way to do it. Actually, I know for a fact that he totally knew a better way, but the guy was a massive troll, so there you go.

    Anyway, I managed to replace that monstrosity with the following.

      //get the values of all the currently applied discounts
      var discounts = new[]
      {
          new 
          { 
              DiscountType = Discounts.LocalResident,
              Discount = LocalResident ? LocalResidentDiscount : 0m,
          },
          new 
          { 
              DiscountType = Discounts.Amabassador,
              Discount = ResortAmbassador ? ResortAmbassadorDiscount : 0m,
          },
          new 
          {
              DiscountType = Discounts.SpecialOffer,
              Discount = UseSpecialOffer ? SpecialOfferDiscount : 0m,
          },
          new 
          {
              DiscountType = Discounts.DiscountCode,
              Discount = DiscountCodeDiscount,
          },
      };
    
      //get the discount with the highest value
      var maxDiscount = discounts.OrderByDescending(d => d.Discount).First();
      _CurrentDiscount = maxDiscount.DiscountType;
    
      //if there isn't a discount applied
      if (maxDiscount.Discount == 0)
      {
          //set the current discount type to none
          _CurrentDiscount = Discounts.None;
      }
    

    Now onto the next WTF…


    Filed nuder: I'm sure there will be more to come...


  • Why did you use OrderByDescending(d => d.Discount).First() instead of Max(d => d.Discount)?


  • Discourse touched me in a no-no place

    Because Max will only return the discount specified in the selector (in this case a decimal). I need the whole object (the discount and the discount type).



  • Oops, I see that now.



  • Is at least one of those conditions guaranteed to apply? If no discounts apply, First() will throw.

    With some extra helper methods/types, you might have been able to write something like:

    discounts.Where(x => x.AppliesTo(customer)).MaxByMaybe(x => x.Value).Select(x => x.DiscountType)
    

    The result would be Maybe<DiscountType>.

    where

    struct Maybe<A>
    {
        Maybe(A val) { HasValue = true; Value = val; }
        bool HasValue;
        private A Value;
        Maybe<B> Select<B>(Func<A, B> f)
        {
            return HasValue ? new Maybe<B>(f(Value)) : new Maybe<B>();
        }
    }
    
    Maybe<A> MaxByMaybe<A, B>(this IEnumerable<A> e, Func<A, B> f) where B : IComparable<B>
    {
        if e is empty return new Maybe<B>()
        return new Maybe<B>(MaxBy(e, f));
    }
    

    Got carried away and then lazy there at the end. whatever


  • Discourse touched me in a no-no place

    Nice idea, but it's not necessary in this case. discounts is a hard coded array, so First is guaranteed to always return something. Otherwise I'd have used FirstOrDefault and added a null check.


  • Discourse touched me in a no-no place

    @Bort said:

    Is at least one of those conditions guaranteed to apply? If no discounts apply, First() will throw.

    Easier to just put a “no discount” Discount in.



  • @DoctorJones said:

    discounts is a hard coded array

    Oh, right. derp. I was thinking of my plan of using Where().

    @dkf said:

    Easier to just put a “no discount” Discount in.

    "Easier" != "a good idea". Not having a discount is a different type of result from having a discount (and it is a LocalResident discount, for example). A type like Maybe is useful for making this distinction, even if a bit vague (why is it maybe?).

    On a similar note, it's always bothered me that a function like int String.IndexOf(String s) returns -1 for "not found". "Not Found" is a different type of result from "Found and it's at index 5". Since -1 is the same type as an index, following code could easily overlook the possibility of the missing value and get funky results.

    example: someString.Substring(someString.IndexOf("#!") + 2)

    we want everything after the first shebang. If there is no shebang, we get all but the first character of the input string.

    also, using -1, null, NoDiscount, etc. makes us use multiple representations for missing where a simple type like Maybe gives us one, and a single set of convenient methods for working with missing values.

    also, null's fold in the sense that you can't have multiple levels of null or multiple reasons for why something is null while you could have multiple levels of Maybe or some similar, more context-specific, type.

    dictated but not read


  • I survived the hour long Uno hand

    @DoctorJones said:

    Because Max will only return the discount specified in the selector (in this case a decimal). I need the whole object (the discount and the discount type).

    If the sorting approach is ever a bad idea, you could try something like this.

    var maxDiscount = discounts.Aggregate((highest, next) => (highest.Discount > next.Discount) ? highest : next);
    

    It comes with a tradeoff in terms of how easy it is to understand at a glance, though that could be improved with a little effort.



  • In Haskell, I'd define a type of discounts and make it an Ord instance and then make a set of available discounts (possibly dynamically, if that's required) and then query the largest one.



  • I think you just reinvented Nullable...


  • Discourse touched me in a no-no place

    @Maciejasjmj said:

    I think you just reinvented Nullable...

    I think he was trying to achieve something similar, but Nullable only works for value types. He wanted something else that would allow him to distinguish between null and "no discount".

    @Bort said:

    null's fold in the sense that you can't have multiple levels of null or multiple reasons for why something is null while you could have multiple levels of Maybe or some similar, more context-specific, type.



  • @Maciejasjmj said:

    I think you just reinvented Nullable...

    Nullable only works for structs.

    And it looks like this can be done: ((Nullable<int>) null) + 1 (quick dotnetfiddling)

    Which has the same problems I was talking about up-thread.

    @DoctorJones said:

    I think he was trying to achieve something similar, but Nullable only works for value types. He wanted something else that would allow him to distinguish between null and "no discount".

    :hanzo:



  • Well duh, reference types are nullable by default. As for "multiple reasons for null", I can't quite see how your code solves that.

    Null, together with ?? and ?. operators pretty much covers 99 percent of the "this value is missing" cases - and the remaining 1 percent is likely to be so specific that you won't be able to genericize that.



  • @DoctorJones said:

    this.DiscountCodeDiscount

    Such discount. Wow.


  • Discourse touched me in a no-no place

    @Shoreline said:

    Such discount. Wow.

    I cannot emphasise this enough; Not. My. Code.

    💩



  • @DoctorJones said:

    Anyway, I managed to replace that monstrosity with the following.

      //get the values of all the currently applied discounts
      var discounts = new[]
      {
          new 
          { 
              DiscountType = Discounts.LocalResident,
              Discount = LocalResident ? LocalResidentDiscount : 0m,
          },
          new 
          { 
              DiscountType = Discounts.Amabassador,
              Discount = ResortAmbassador ? ResortAmbassadorDiscount : 0m,
          },
          new 
          {
              DiscountType = Discounts.SpecialOffer,
              Discount = UseSpecialOffer ? SpecialOfferDiscount : 0m,
          },
          new 
          {
              DiscountType = Discounts.DiscountCode,
              Discount = DiscountCodeDiscount,
          },
      };
    
      //get the discount with the highest value
      var maxDiscount = discounts.OrderByDescending(d => d.Discount).First();
      _CurrentDiscount = maxDiscount.DiscountType;
    
      //if there isn't a discount applied
      if (maxDiscount.Discount == 0)
      {
          //set the current discount type to none
          _CurrentDiscount = Discounts.None;
      }
    

    Now onto the next WTF…


    Filed nuder: I'm sure there will be more to come...

    I must be missing something, because I don't understand how what you have is clearer or simpler or more obvious or straightforward or maintainable or performant than

    _CurrentDiscount = Discounts.None;
    var maxDiscount = 0;
    
    if (LocalResident && LocalResidentDiscount > maxDiscount) {
        _CurrentDiscount = Discounts.LocalResident;
        maxDiscount = LocalResidentDiscount;
    }
    if (ResortAmbassador && ResortAmbassadorDiscount > maxDiscount) {
        _CurrentDiscount = Discounts.ResortAmbassador;
        maxDiscount = ResortAmbassadorDiscount;
    }
    if (UseSpecialOffer && SpecialOfferDiscount > maxDiscount) {
        _CurrentDiscount = Discounts.SpecialOffer;
        maxDiscount = SpecialOfferDiscount;
    }
    if (DiscountCodeDiscount > maxDiscount) {
        _CurrentDiscount = Discounts.DiscountCode;
        maxDiscount = DiscountCodeDiscount;
    }
    
    
    

  • Discourse touched me in a no-no place

    This post is deleted!

  • Discourse touched me in a no-no place

    Yeah, that's another way of approaching it.

    I personally think this is clearer than a bunch of if statements

    //get the discount with the highest value
    var maxDiscount = discounts.OrderByDescending(d => d.Discount).First();
    

    but yours is more performant.



  • @DoctorJones said:

    I personally think this is clearer

    It certainly would be, if the design already included the discounts array instead of the assortment of individual variables. As things stand now, though, I think the need to build discounts on-the-fly negates any extra clarity that your subsequent ability to apply OrderByDescending().First() gets you.

    But you're quite right that anything is better than the pile-o- :wtf: you were handed to begin with.


  • Discourse touched me in a no-no place

    I agree, building discounts on the fly isn't desirable. I did it that way in anticipation of the discounts coming from the CMS. When/if that day comes, this code will be compatible with less effort.


  • Considered Harmful

    Discounts. One day, one day, I will tell you about discounts. One day you will know what "simple" and "complex" signify in the face of an utter abomination from before the time when the world was as the mind of man can know.


Log in to reply