Dynamically update attribute of a same name in different classes

Posted on

Problem

I have this piece of code which I know can be improved greatly, but I simply don’t know how. I’ve been googling for a little while, but truth to be told, I’m not really sure what I should be even looking for.

As you can see the code is very straight forward. Different classes have attributes of a same name, but I can’t figure out how to dynamicaly swap between classes.

Please help me make it more dynamic, make it cleaner!!! Any help is much appreciated.

public void setHorsePurchaseInvoiceStatus(TransactionLine entity)
{
    if (entity.HorsePurchase != null)
    {
        switch (entity.Section)
        {
            case ("PI1"):
                entity.HorsePurchase.StatusPI1 = getCurrentSectionStatus(entity) ?? entity.HorsePurchase.PI1;
                break;
            case ("PI2"):
                entity.HorsePurchase.StatusPI2 = getCurrentSectionStatus(entity) ?? entity.HorsePurchase.PI2;
                break;
            case ("PI3"):
                entity.HorsePurchase.StatusPI3 = getCurrentSectionStatus(entity) ?? entity.HorsePurchase.PI3;
                break;
            case ("PI4"):
                entity.HorsePurchase.StatusPI4 = getCurrentSectionStatus(entity) ?? entity.HorsePurchase.PI4;
                break;
            case ("SI1"):
                entity.HorsePurchase.StatusSI1 = getCurrentSectionStatus(entity) ?? entity.HorsePurchase.SI1;
                break;
            case ("SI2"):
                entity.HorsePurchase.StatusSI2 = getCurrentSectionStatus(entity) ?? entity.HorsePurchase.SI2;
                break;
        }
    }
    else if (entity.HorseSale != null)
    {
        switch (entity.Section)
        {
            case ("PI1"):
                entity.HorseSale.StatusPI1 = getCurrentSectionStatus(entity)?? entity.HorseSale.StatusPI1;
                break;
            case ("PI2"):
                entity.HorseSale.StatusPI2 = getCurrentSectionStatus(entity)?? entity.HorseSale.StatusPI2;
                break;
            case ("PI3"):
                entity.HorseSale.StatusPI3 = getCurrentSectionStatus(entity)?? entity.HorseSale.StatusPI3;
                break;
            case ("PI4"):
                entity.HorseSale.StatusPI4 = getCurrentSectionStatus(entity)?? entity.HorseSale.StatusPI4;
                break;
            case ("SI1"):
                entity.HorseSale.StatusSI1 = getCurrentSectionStatus(entity)?? entity.HorseSale.StatusSI1;
                break;
            case ("SI2"):
                entity.HorseSale.StatusSI2 = getCurrentSectionStatus(entity)?? entity.HorseSale.StatusSI2;
                break;
            case ("SI3"):
                entity.HorseSale.StatusSI3 = getCurrentSectionStatus(entity)?? entity.HorseSale.StatusSI3;
                break;
        }
    }
    else if (entity.HorseDivideProfit != null)
    {
        switch (entity.Section)
            {
                case ("PI1"):
                    entity.HorseDivideProfit.StatusPI1 = getCurrentSectionStatus(entity) ?? entity.HorseDivideProfit.StatusPI1;
                    break;
                case ("SI1"):
                    entity.HorseDivideProfit.StatusSI1 = getCurrentSectionStatus(entity) ?? entity.HorseDivideProfit.StatusSI1;
                    break;
            }
    }
}

Solution

For one thing, getCurrentSectionStatus(entity) is duplicated multiple times. It would be better to put that in a local variable and use that variable instead.
That way if at some point you change the way you get the current section status,
you can make the change in one place.

However,
there are clearly deeper problems with this code.
Most noticeably,
field names such as StatusPI1, StatusPI2, …, and StatusSI1, StatusSI2, …, stink, as variable names containing numbers usually do.

Translated to pseudocode, the method looks like this:

  1. if the TransactionLine contains a HorsePurchase: update a field in it depending on the section field of the line
  2. or else, if the TransactionLine contains a HorseSale: update a field in it depending on the section field of the line
  3. or else, if the TransactionLine contains a HorseDivideProfit: update a field in it depending on the section field of the line

At the minimum, it would be better to have separate TransactionLine subclasses to handle each type, for example HorsePurchaseTransaction, HorseSaleTransaction, HorseDivideProfitTransaction.
Then you could have an HorsePurchase.update method that takes a HorsePurchaseTransaction as parameter and apply the necessary changes,
and do similarly for the other types too.

This is still not enough to treat the odd StatusPI1, StatusPI2, and other fields, but just based on the posted code, I don’t think there is enough information to propose an alternative approach. After splitting the TransactionLine as I described above, you could post the revised version and some more of the rest of the code in a new question,
so that we can address these issues and suggest further improvements.

Leave a Reply

Your email address will not be published. Required fields are marked *