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:
- if the
TransactionLine
contains aHorsePurchase
: update a field in it depending on the section field of the line - or else, if the
TransactionLine
contains aHorseSale
: update a field in it depending on the section field of the line - or else, if the
TransactionLine
contains aHorseDivideProfit
: 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.