1

Im working on a e-commerce sort of application.

Entities are:

  • Product with List<Expense> Expenses
  • Expense with Description props which is referring to packaging, transport, administrative costs etc.

I need to have at least 2 types of expenses:

  1. Relative expense (amount in percentage, calculated off the product price) and
  2. Absolute expense (amount in numbers)

What i tried doing is having an

  • abstract class Expense with props: Id, Description.
  • class RelativeExpense : Expense with AmountInPercentage props and
  • class AbsoluteExpense : Expensewith Amount props.

For calculating expenses I'm having a GetTotalExpenseAmount(Product p) method:

public decimal GetTotalExpenseAmount(Product p)

{
  decimal totalExpenses = 0;
  foreach (var expense in p.Expenses)
  {
       if(expense.GetType() == typeof(RelativeExpense))
       {
           totalExpenses += p.BasePrice * (expense as RelativeExpense).AmountInPercentage / 100;
       }
       else if(expense.GetType() == typeof(AbsoluteExpense))
       {
          totalExpenses += (expense as AbsoluteExpense).Amount;
       }
                
  }

    return totalExpenses;
}

My question is, is this a good practice? Reason i'm asking is because AS operator is doing casting and i know that can be expensive performance-wise. Plus i will have a Logger which will print out all expanses for a single product and therefore this foreach with as operator will be used again.

Is there any other way i can achieve what i want, but with better/cleaner code and performance? Maybe different modeling? Any ideas how should i approach this?

Context
  • 53
  • 5

3 Answers3

4

Why casting can be expensive performance wise? I don't think so.

However, you can use the is operator here which also skips nulls:

foreach (var expense in p.Expenses)
{
   if(expense is RelativeExpense relativeExpense)
   {
       totalExpenses += p.BasePrice * relativeExpense.AmountInPercentage / 100;
   }
   else if(expense is AbsoluteExpense absoluteExpense)
   {
      totalExpenses += absoluteExpense.Amount;
   }         
}
Tim Schmelter
  • 450,073
  • 74
  • 686
  • 939
  • 1
    A switch expression would remove the need to check the type twice. It would also make it easier to find missed cases – Panagiotis Kanavos Apr 19 '21 at 10:14
  • @PanagiotisKanavos: You mean twice because of the `is` in the `if` and the `is` in the `else if`? Are you sure that it's different with a [`switch` type pattern](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/switch#pattern-matching-with-the-switch-statement)? The type is not known at compile time. – Tim Schmelter Apr 19 '21 at 10:17
  • 1
    Yes, but I'm more concerned with missed cases. A default `_=>0` makes it clear that any missed cases won't contribute. It's also easier to see what's handled compared to the original code that used `GetType()` – Panagiotis Kanavos Apr 19 '21 at 10:18
  • PS: That's why I'm anxiously waiting for DUs, and getting rather ... annoyed (to put it mildly) that they seem to be postponed yet again – Panagiotis Kanavos Apr 19 '21 at 10:20
  • @PanagiotisKanavos What are DUs? – 41686d6564 stands w. Palestine Apr 19 '21 at 10:21
  • 1
    @41686d6564 "Discriminated Union", see https://github.com/dotnet/csharplang/issues/113 and https://github.com/dotnet/csharplang/blob/main/proposals/discriminated-unions.md – Matthew Watson Apr 19 '21 at 10:24
  • 2
    @41686d6564 with a discriminated union the compiler itself would detect whether a case is missing. This means that if you added eg a `SpeculativeExpense` afterwards you'll get an error telling you it's not handled in `GetTotalExpenseAmount`. And vice-versa, you won't have to use a default clause when you *do* handle all cases – Panagiotis Kanavos Apr 19 '21 at 10:29
  • @PanagiotisKanavos Can you provide a code with your solution with `switch case`? Not sure what should be in `switch` and what should be in `case` part of the code. – Context Apr 19 '21 at 11:13
2

You can uses the OOO Polymorphism. So the C# virtual/abstract methods.

public decimal GetTotalExpenseAmount(Product p)
{
  decimal totalExpenses = 0;
  foreach (var expense in p.Expenses)
  {
       totalExpenses += expense.GetPrice();
  }
  return totalExpenses;
}

public class Product {
    public string Name;
    public List<Expense> Expenses;
}

public abstract class Expense {
    public string Id;
    public string Description;
    
    public abstract int GetPrice();
}

public class RelativeExpense : Expense {
    public int AmountInPercentage;
    public int BasePrice;
    
    public override int GetPrice() {
        return BasePrice * AmountInPercentage / 100;
    }
}

public class AbsoluteExpense : Expense { 
    public int Amount;
    
    public override int GetPrice() {
        return Amount;
    }
}

One caveat I moved the BasePrice to RelativeExpense class.

lukaszberwid
  • 1,097
  • 7
  • 19
  • This doesn't give the same result since the relative expense requires knowing the product base price. I realise you have just moved that property into the expense object, but that is unlikely to be a good idea. For example, let's say you are storing the relative expense object in your DB as a tax increase. You may want to use the object against multiple products, all with different base prices. – DavidG Apr 19 '21 at 10:30
  • This is one case where polymorphism is a problem, not a solution. Who says `RelativeExpense` refers to a product's base price? – Panagiotis Kanavos Apr 19 '21 at 10:30
  • true, that was my assumption that it refers to base price. If it can't be moved then solution proposed by @Tim Schmelter is the way to go – lukaszberwid Apr 19 '21 at 10:33
  • Actually, I'm not a fan of Tim's answer either, would much rather see a switch expression as suggested by @Panagiotis. My rule of thumb is generally - if there is an `else` statement, then there's likely a better way to write that code. – DavidG Apr 19 '21 at 10:35
0

I'd suggest a single class with IsRelative property:

class Expense 
{
    (...)
    public bool IsRelative {get;}
    public decimal AmountInPercentage {get;}
    public decimal Amount {get;
}
(...)
foreach (var expense in p.Expenses)
{
    if(expense.IsRelative)
    {
        totalExpenses += p.BasePrice * expense.AmountInPercentage / 100;
    }
    else
    {
        totalExpenses += (expense as AbsoluteExpense).Amount;
    }            
}

or even

class Expense 
{
    (...)
    public bool IsRelative {get;}
    public decimal Amount {get;}
}

(...)

foreach (var expense in p.Expenses)
{
    if(expense.IsRelative)
    {
        totalExpenses += p.BasePrice * expense.Amount / 100;
    }
    else
    {
        totalExpenses += expense.Amount;
    }            
}
tymtam
  • 31,798
  • 8
  • 86
  • 126