• some guy (unregistered)

    But be sure to check if it is a Saturday frist, and then forget about it.

  • (nodebb)

    It's just a slightly misnamed function. The sh in shCharge means "Shipping and Handling", so they can charge the cost of shipping plus whatever amount they feel like for "handling".

    Next you're going to tell me that passing the shipping types around as stringly typed data instead of enums is a mistake, too!

    No, next time I'm going to tell you that using double for money is a mistake. After that, I'm going to tell you that checking if the subtotal is greater than 0 is a bit weird.

  • HC (unregistered)

    I'm not seeing it :-( .

  • Jaroslav (unregistered)

    Pretty sure someone saw this code, and said "Just ship it".

  • JPJ (unregistered)

    And if this wasn't true, what would it be then? FileNotFound?

  • (nodebb)

    Now that one looks like Java code because it lacks proper switch pattern matching :D

  • (nodebb) in reply to MaxiTB

    It's not Java String.equals() has a lower case "e" in Java.

  • (nodebb) in reply to jeremypnet

    That's a good point - so what is it, because the method name is lower case, so unlikely to be C# for example. Plus the code would like this:

    public static double GetShippingCharge(string shippingType, double subTotal) =>
	shippingType switch
	{
		"Ground" => subTotal switch
		{
			<= 29.99 => 4.95,
			<= 99.99 => 7.95,
			<= 299.99 => 9.95,
			> 299.99 => subTotal * .05,
			_ => 0
		},
		"Two-Day" => subTotal switch
		{
			<= 29.99 => 14.95,
			<= 99.99 => 19.95,
			<= 299.99 => 29.95,
			> 299.99 => subTotal * .10,
			_ => 0
		},
		"Next Day" => subTotal switch
		{
			<= 29.99 => 24.95,
			<= 99.99 => 34.95,
			<= 299.99 => 44.95,
			> 299.99 => subTotal * .15,
			_ => 0
		},
		"Next Day a.m." => subTotal switch
		{
			<= 29.99 => 29.95,
			<= 99.99 => 39.95,
			<= 299.99 => 49.95,
			> 299.99 => subTotal * .2,
			_ => 0			
		},
		_ => 0
	};
  • Mark (unregistered)

    Honestly, if the company is fairly flexible on deploying code updates within a few days, this isn't all that bad, especially with having different ways. of calculating shipping on totals from 300 up. At least they're not developing a hugely abstracted calculation engine that is driven by 15 database tables that only the original developer understands how to fill. It's simple to read, understand, and maintain.

    Still, a fair point about stringly vs enums.

  • retsep (unregistered)

    For me it seems like an awkward attempt to charge for swiftness. The function should have three constants and part of price, as well as use of function argument saturday.

    public double getShippingCharge(int shippingType, bool saturday, int subTot) { int shCharge = 0; if (subTot>0) { if(shippingType==ENUM_TYPE_GROUND) { shCharge = (int)round(495+subTot * .05); } else if(shippingType==ENUM_TYPE_TWO_DAY) { shCharge = (int)round(1495+subTot * .10); } else if(shippingType==ENUM_TYPE_NEXT_DAY) { shCharge = (int)round(2495+subTot * .15); } else if(shippingType==ENUM_TYPE_NEXT_DAY_AM) { shCharge = (int)round(2995+subTot * .20); }
    }
    return shCharge; } Handling of saturday is left to reader.

  • Hanzito (unregistered) in reply to Mark

    TBF, this would be a single lookup in a single table. The code is structured like a very simple SELECT.

  • TS (unregistered)

    You're confusing price (what you charge the customer) with cost (what the shipping company charges you). The business has clearly decided that the price they charge for shipping should be a function of price of the goods purchased, which will hopefully balance out the cost overall, rather than trying to set the price equal to the cost for every single order. This is common and makes things simple for the customer. The business absorbs swings in the cost of shipping, and just needs to check periodically that they are still making a profit overall. The code doesn't need to be changed more than every few months, probably no more than annually.

  • (nodebb)

    Ya'll think this is a massive WTF but it has given me a great idea: instead of hard-coding the breakpoints directly in the source, I'll invent a place external to the application where the values can be stored, and then read in when needed. I will call this innovation a "database"!

  • (nodebb)

    "They paid me to write a function to calculate shipping costs. They never said anything about updating the calculation if the costs ever change. I'm either too dumb or too apathetic to do the right thing."

  • AzureDiamond (unregistered) in reply to MaxiTB

    its c# just a code style thats so awful you might think its another language. maybe somebody read private fields should be lower case and thought it was the same for private methods

  • (nodebb)

    I'm with TS, this isn't a WTF at all. This is how many small online retailers calculate shipping and handling. There are some I deal with that actually give every item in the catalog a shipping value and add them all up, but most just give flat amounts for ranges of subtotal. Many even do free (instead of the 20% here) above a certain threshhold. Now I can't say this is the best way to logically lay out the calculations, but the concept is fine. Some of the sites I frequent have been known to go years between shipping price changes.

  • (nodebb)

    TBH, these rules are so custom that this approach actually lists them in a way that works for whoever's interested in learning them. It's either this, or having to go into Shopify (or whatever) and try to make sense of the rules there.

    And don't get me started about tax calculations!

  • Duke of New York (unregistered)

    TRWTF is writing range conditions in dominant-sided style, and even with the upper bound first.

  • (nodebb)

    The fact they have the bool "saturday" but aren't using it yet, shows they have given thought to keeping the code future proof!

  • (nodebb) in reply to Duke of New York

    I second that. Who in their right mind writes range checks in the order "a < 5 && a >= 1" when clearly the natural order "a >= 1 && a < 5" is so much easier to read?

    That's TRWTF. The functionality itself looks like whatever a myriad of e-commerce sites are already doing. Sure it could be more fancy (a table, a lookup table, a db, etc) but sometimes just getting the job done and moving on is a good strategy.

    If you end up having to edit it this every over day, them sure, make it better -- the obvious solution is to COM interop that and read it from an Excel spreadsheet ;-)

  • (nodebb) in reply to Ralf

    Sure it could be more fancy (a table, a lookup table, a db, etc)

    Let's analyze those choices (with the assumption it's C# and using my proper implementation of the same code with pattern matching):

    1. table

    I honestly don't know what this means in context with C#. Table is a construct generally referred to databases, there is a DataTable in C# but that one is an ancient legacy type that should no longer be used. So if you are referring to this one, it's not a good idea because it belonged to an outdated way to interact with databases. The modern way is to use ORMs like EntityFrameworkCore or Dapper (see 3).

  • (nodebb) in reply to Ralf
    1. lookup table

    I think you are referring to in-memory structures like Dictionary<TK,TV>. Yes, this an option, but there are three major downsides to this approach:

    • First you have to created either multiple dictionary or a dictionary containing dictionaries. That's a lot of abstraction and really doesn't make the code more readable, it's pretty objectively worse. Also you now have to work with ranges or operation codes to actually know how to work with those stored values which decreases readability even more. You have not split the code over different locations greatly decreasing readability. Not to mention the implementation will get way, way more complex, increasing cost of implementation and maintenance.

    • Testing is a major issue: You have now technically introduced generic code, which requires a lot more testing effort instead of just testing every execution branch.

    • Finally in .net performance for switch statements is pretty straight forward: Don't use Dictionary<TK,TV> if you have less than 10 cases. It's more complex, because technically the compiler generates different code for switch statements depending the amount of execution branches, but it's pretty obvious if you think about it: Hash table lookups are actually pretty expensive compared to operations that only cost a few machine cycles and will be parallelized by all modern CPUs nicely because of branch prediction.

  • Duke of New York (unregistered) in reply to Ralf
    Comment held for moderation.
  • (nodebb) in reply to Ralf
    1. database

    This is by far the worst idea - not only is this the most generic way to implement this problem, now the matter of who can access the database becomes a factor. So basically you introduced hardware dependencies plus concurrency considerations - and if you just ignore them, you introduced not only a lot of technical debt plus also a ton of possibly undefined states. Not to mention the testing goes up to a whole level - what was before a dozen of tests to completely cover this feature is now pretty much an endless amount that is required from non-existing tables, connection and timeout issues, db mapping issues, mentioned concurrency issues and making sure that all the business cases are consistent while the data is changed, checking value ranges, checking inconsistencies, making sure the values can only be changed by authorized actors... the list goes on to absurd levels for such a simple feature.

    So yeah, it's pretty straight forward, as a developer you want to generated business value - nobody will give you a cookie for creating the most abstract generic approach. It should be clean code following best practices, but this also means considering the whole vertical slice from maintainability, testing deployment and actual live environment behavior.

    Addendum 2025-04-11 14:37: There would be actually a total of 21 tests required to test the reference implementation (I remembered it to be smaller). But still it's a far cry from the potential hundreds of tests required for a database version.

  • airdrik (unregistered)
    Comment held for moderation.
  • Officer Johnny Holzkopf (unregistered) in reply to Bananafish
    Comment held for moderation.

Leave a comment on “A Steady Ship”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article