Client trip type contracts

I want to make this code generic, so that I could remove if/else and recursive for loops.

I am open to accept all kind of suggestions regarding common interface implementation but kindly check that their property names FKID are different, and as these are POCO entities I can't change the proper names.

var newClientTripTypeContracts = newClientTripTypeContract.ClientTripTypeContractChargeValueLists.Where(u => u.ClientTripTypeContractChargeValueListID == 0).ToList();

    int counter = -1;
    for (int i = 0; i  0)
        {
            for (int j = 0; j  0)
        {
            for (int j = 0; j  0)
        {
            for (int j = 0; j  0)
        {
            for (int j = 0; j < newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeTierMONEYValues.Count; j++)
            {
                newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeTierMONEYValues[j].ChargeTierMONEYValueFKID = counter;
            }                 
        }                
        counter -= 1;
    }

Bug? In each of your 4 inner for loops, your code is referencing the following:

newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i]

Shouldn't it be this, instead?

newClientTripTypeContracts[i]

EDIT: I missed the OP's restriction in the question that indicated that property names couldn't be changed. I've added another section at the bottom to attempt to modify my original answer to accomodate this restriction.

Making some assumptions about your data structures, I'd try consolidating the code with the following approach:

-
Since it seems like your ChargeBITValues, ChargeDECIMALValues, ChargeENUMValues, and ChargeTierMONEYValues lists contain similar-but-different objects, I would first ensure that they are related by deriving from a common base class. I would define the heirarchy as follows:

abstract class ChargeValue
{
    public abstract int ChargeValueFKID { get; set; }
}

class ChargeBITValue : ChargeValue
{
    // TODO: implement the ChargetValueFKID property
}

class ChargeDECIMALValue : ChargeValue
{
    // TODO: implement the ChargetValueFKID property
}

class ChargeENUMValue : ChargeValue
{
    // TODO: implement the ChargetValueFKID property
}

class ChargeTierMONEYValue : ChargeValue
{
    // TODO: implement the ChargetValueFKID property
}

Notice that the base class has an abstract ChargeValueFKID property. This property can be overridden in each derived class, and is intended to replace or work with the ChargeBITValueFKID, ChargeDECIMALValueFKID, ChargeENUMValueFKID, and ChargeTierMONEYFKID properties that are in your original code.

-
Next, I would create an "ApplyCharges" method to perform the same task as in each of your 4 inner for loops. You could define it as something like the following. Alternatively, you could make this a public instance method of the ClientTripTypeContract class:

static bool ApplyCharges(ClientTripTypeContract contract, IList charges, int fkid)
{
    if (charges == null || charges.Count == 0)
        return false;

    foreach (var charge in charges)
    {
        charge.ChargeValueFKID = fkid;
    }

    return true;
}

-
Finally, I'd update your original code to the following:

var newClientTripTypeContracts = newClientTripTypeContract.ClientTripTypeContractChargeValueLists.Where(u => u.ClientTripTypeContractChargeValueListID == 0).ToList();

    int counter = -1;
    foreach(var contract in newClientTripTypeContracts)
    {
        contract.ClientTripTypeContractChargeValueListID = counter;

        bool chargesApplied = false;

        if (!chargesApplied)
            chargesApplied = ApplyCharges(contract, contract.ChargeBITValues, counter);
        if (!chargesApplied)
            chargesApplied = ApplyCharges(contract, contract.ChargeDECIMALValues, counter);
        if (!chargesApplied)
            chargesApplied = ApplyCharges(contract, contract.ChargeENUMValues, counter);
        if (!chargesApplied)
            chargesApplied = ApplyCharges(contract, contract.ChargeTierMONEYValues, counter);

        counter -= 1;
    }

Hopefully this will at least get you started and maybe give you a few ideas. You could probably come up with a cleaner or more specific approach that makes more sense, since you have the knowledge around what these data structures mean.

EDIT:
You could, perhaps simplify your data structures with the following changes:

-
Add a ChargeType property to the ChargeValue base class. Create a ChargeType enum with the following values: Bit, Decimal, Enum, and TierMoney.

enum ChargeType
{
    Bit,
    Decimal,
    Enum,
    TierMoney,
}

abstract class ChargeValue
{
    public abstract ChargeType ChargeType { get; }

    public abstract int ChargeValueFKID { get; set; }
}

-
Then, Instead of having the 4 properties ChargeBITValues, ChargeDECIMALValues, ChargeENUMValues, and ChargeTierMONEYValues, you could simply have 1 property called ChargeValues, which contains all of the charge values from all of those properties. You could even keep those 4 original properties, but I would make them read-only and have them return an IEnumerable object; adding/removing charge values should be done to the ChargeValues property. An example is shown below:

```
class ClientTripTypeContract
{
// ChargeValues replaces the original ChargeBITValues, ChargeDECIMALValues, ChargeENUMValues, and ChargeTierMONEYValues properties
public IList ChargeValues { get; private set; }

public IEnumerable ChargeBITValues
{
get { return this.ChargeValues.Where(charge => charge.ChargeType == ChargeType.Bit).Cast(); }
}

public IEnumerable ChargeDECIMALValues
{
get { return this.ChargeValues.Where(charge => charge.ChargeType == ChargeType.Decimal).Cast(); }
}

public IEnumerable ChargeENUMValues
{
get { return this.ChargeValues.Where(charge => charge.ChargeType == ChargeType.Enum).Cast(); }
}

public IEnumerable ChargeTierMONEYVa

StackExchange Code Review Q#7417, answer score: 5