3
\$\begingroup\$

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 < newClientTripTypeContracts.Count; i++)
    {
        newClientTripTypeContracts[i].ClientTripTypeContractChargeValueListID = counter;
        if (newClientTripTypeContracts[i].ChargeBITValues.Count > 0)
        {
            for (int j = 0; j < newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeBITValues.Count; j++)
            {
                newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeBITValues[j].ChargeBITValueFKID = counter;
            }      
        }
        else if (newClientTripTypeContracts[i].ChargeDECIMALValues.Count > 0)
        {
            for (int j = 0; j < newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeDECIMALValues.Count; j++)
            {
                newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeDECIMALValues[j].ChargeDECIMALValueFKID = counter;
            }  
        }
        else if (newClientTripTypeContracts[i].ChargeENUMValues.Count > 0)
        {
            for (int j = 0; j < newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeENUMValues.Count; j++)
            {
                newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeENUMValues[j].ChargeENUMValueFKID = counter;
            }                
        }              
        else if (newClientTripTypeContracts[i].ChargeTierMONEYValues.Count > 0)
        {
            for (int j = 0; j < newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeTierMONEYValues.Count; j++)
            {
                newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeTierMONEYValues[j].ChargeTierMONEYValueFKID = counter;
            }                 
        }                
        counter -= 1;
    }
\$\endgroup\$
1
  • \$\begingroup\$ I would start by extracting newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i] and newClientTripTypeContracts[i] to make the loop easier to read. \$\endgroup\$ Jan 3, 2012 at 13:35

3 Answers 3

5
\$\begingroup\$

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<ChargeValue> 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<ChargeValue> 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<ChargeValue> ChargeValues { get; private set; }
    
        public IEnumerable<ChargeBITValue> ChargeBITValues
        {
            get { return this.ChargeValues.Where(charge => charge.ChargeType == ChargeType.Bit).Cast<ChargeBITValue>(); }
        }
    
        public IEnumerable<ChargeDECIMALValue> ChargeDECIMALValues
        {
            get { return this.ChargeValues.Where(charge => charge.ChargeType == ChargeType.Decimal).Cast<ChargeDECIMALValue>(); }
        }
    
        public IEnumerable<ChargeENUMValue> ChargeENUMValues
        {
            get { return this.ChargeValues.Where(charge => charge.ChargeType == ChargeType.Enum).Cast<ChargeENUMValue>(); }
        }
    
        public IEnumerable<ChargeTierMONEYValue> ChargeTierMONEYValues
        {
            get { return this.ChargeValues.Where(charge => charge.ChargeType == ChargeType.TierMoney).Cast<ChargeTierMONEYValue>(); }
        }
    }
    


According to the OP's question, the properties of the data structure classes can't be changed/renamed, so below is my attempt to take the suggestions mentioned above and still work with that restriction:

Below, I've removed the ChargeValue base class and replaced it with an IChargeValue interface. You can have each Charge*Value class implement this interface. In the example code below, I've implemented that interface explicitly so that the properties of that interface do not show up as public properties; I don't know if that matters...perhaps an OR mapper tool that you are using looks at the public properties. Alternatively, there ought to be some mechanism that your OR mapper uses that allows you to ignore these properties.

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

    interface IChargeValue
    {
        ChargeType ChargeType { get; }

        int ChargeValueFKID { get; set; }
    }

    class ChargeBITValue : IChargeValue
    {
        int ChargeBITValueFKID { get; set; }

        ChargeType IChargeValue.ChargeType
        {
            get { return ChargeType.Bit; }
        }

        int IChargeValue.ChargeValueFKID
        {
            get { return this.ChargeBITValueFKID; }
            set { this.ChargeBITValueFKID = value; }
        }
    }

    class ChargeDECIMALValue : IChargeValue
    {
        int ChargeDECIMALValueFKID { get; set; }

        ChargeType IChargeValue.ChargeType
        {
            get { return ChargeType.Decimal; }
        }

        int IChargeValue.ChargeValueFKID
        {
            get { return this.ChargeDECIMALValueFKID; }
            set { this.ChargeDECIMALValueFKID = value; }
        }
    }

    class ChargeENUMValue : IChargeValue
    {
        int ChargeENUMValueFKID { get; set; }

        ChargeType IChargeValue.ChargeType
        {
            get { return ChargeType.Enum; }
        }

        int IChargeValue.ChargeValueFKID
        {
            get { return this.ChargeENUMValueFKID; }
            set { this.ChargeENUMValueFKID = value; }
        }
    }

    class ChargeTierMONEYValue : IChargeValue
    {
        int ChargeTierMONEYValueFKID { get; set; }

        ChargeType IChargeValue.ChargeType
        {
            get { return ChargeType.TierMoney; }
        }

        int IChargeValue.ChargeValueFKID
        {
            get { return this.ChargeTierMONEYValueFKID; }
            set { this.ChargeTierMONEYValueFKID = value; }
        }
    }

Using this approach, you could modify the "ApplyCharges" method mentioned above to the following:

    static bool ApplyCharges(IEnumerable<IChargeValue> charges, int fkid)
    {
        if (charges != null && charges.Any())
        {
            foreach (var charge in charges)
            {
                charge.ChargeValueFKID = fkid;
            }

            return true;
        }
        else
        {
            return false;
        }
    }

And you can call this method using the following:

        // NOTE: the .ToList() call on this line is not required, because the code below uses a foreach loop instead of a for loop.
        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.ChargeBITValues.Cast<IChargeValue>(), counter);
            if (!chargesApplied)
                chargesApplied = ApplyCharges(contract.ChargeDECIMALValues.Cast<IChargeValue>(), counter);
            if (!chargesApplied)
                chargesApplied = ApplyCharges(contract.ChargeENUMValues.Cast<IChargeValue>(), counter);
            if (!chargesApplied)
                chargesApplied = ApplyCharges(contract.ChargeTierMONEYValues.Cast<IChargeValue>(), counter);

            counter -= 1;
        }

Although not used in the example above, you could also add a ChargeValues property to the ClientTripTypeContract class, as shown below:

    class ClientTripTypeContract
    {
        public IList<ChargeBITValue> ChargeBITValues { get; private set; }
        public IList<ChargeDECIMALValue> ChargeDECIMALValues { get; private set; }
        public IList<ChargeENUMValue> ChargeENUMValues { get; private set; }
        public IList<ChargeTierMONEYValue> ChargeTierMONEYValues { get; private set; }

        public IEnumerable<IChargeValue> ChargeValues
        {
            get { return this.ChargeBITValues.Cast<IChargeValue>().Concat(this.ChargeDECIMALValues).Concat(this.ChargeENUMValues).Concat(this.ChargeTierMONEYValues); }
        }
    }

The ChargeValues property just consolidates all of the charges contained in the other properties into a single IEnumerable<IChargeValue> object, so that you can more easily iterate over all of the charges without duplicating code.

The bottom line is that if you want to simplify your code and reduce redundancy, then you will need to somehow add some semantics to your data structures to indicate that the different types of charges are in some ways the same. In my examples, I tried to add those semantics by either inheriting from a common base class or implementing a common interface. If you simply cannot change the code for those classes at all, then another option that would require more work and more refactoring would be to create a layer of abstraction by defining your own wrapper classes. These wrapper classes would encapsulate your existing classes and would expose public properties and methods to allow your application code to manipulate the underlying data.


Update: below is a more simple approach to condensing the original code.

This approach doesn't rely on any data structure changes. This is a bare-bones approach to simplifying the original code.

  • First, define a ForEach extension method. This is just a utility method, so it is not at all specific to this problem:

    public static class EnumerableExtensions
    {
        public static void ForEach<T>(this IEnumerable<T> source, Action<T> action)
        {
            foreach (var item in source)
            {
                action(item);
            }
        }
    }
    
  • Next, update the 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;
    
            if (contract.ChargeBITValues.Any())
                contract.ChargeBITValues.ForEach(charge => charge.ChargeBITValueFKID = counter);
            else if (contract.ChargeDECIMALValues.Any())
                contract.ChargeDECIMALValues.ForEach(charge => charge.ChargeDECIMALValueFKID = counter);
            else if (contract.ChargeENUMValues.Any())
                contract.ChargeENUMValues.ForEach(charge => charge.ChargeENUMValueFKID = counter);
            else if (contract.ChargeTierMONEYValues.Any())
                contract.ChargeTierMONEYValues.ForEach(charge => charge.ChargeTierMONEYValueFKID = counter);
    
            counter -= 1;
        }
    

I don't think this code can be condensed much/any further without modifying the data structures.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for such a great effort, This can't be done as i mentioned these are poco entities and i can't change the property names and use them as you suggested \$\endgroup\$
    – MegaMind
    Jan 3, 2012 at 15:49
  • \$\begingroup\$ @Manvinder - Erm...sorry, I missed that in your question. *grumble grumble* Ok, I understand that you can't change property names; I'm guessing that they map to your database schema. Can you at least add new properties to the classes and leave the existing properties alone? I'll update my answer to try to accomodate this additional restriction. \$\endgroup\$ Jan 3, 2012 at 16:15
  • \$\begingroup\$ Great effort, however at your 5th bullet point, you forgot to change ChargeType.Bit for each respective case to ChargeType.Money, ChargeType.Enum etc \$\endgroup\$
    – Adam
    Jan 5, 2012 at 10:45
0
\$\begingroup\$

I've hoisted a couple of expressions into locals and converted to foreach syntax to better describe intent. It was still not all that readable, so I also extracted each foreach into its own method. I also then replaced .Count > 0 with the LINQ extension method .Any() to also express intent.

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

        var counter = -1;
        var i = 0;
        foreach (var clientTripTypeContractChargeValueList in newClientTripTypeContracts)
        {
            var tripTypeContractChargeValueList = newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i++];

            clientTripTypeContractChargeValueList.ClientTripTypeContractChargeValueListID = counter;
            if (clientTripTypeContractChargeValueList.ChargeBITValues.Any())
            {
                ProcessBITValues(tripTypeContractChargeValueList, counter);
            }
            else if (clientTripTypeContractChargeValueList.ChargeDECIMALValues.Any())
            {
                ProcessDECIMALValues(tripTypeContractChargeValueList, counter);
            }
            else if (clientTripTypeContractChargeValueList.ChargeENUMValues.Any())
            {
                ProcessENUMValues(tripTypeContractChargeValueList, counter);
            }
            else if (clientTripTypeContractChargeValueList.ChargeTierMONEYValues.Any())
            {
                ProcessTierMONEYValues(tripTypeContractChargeValueList, counter);
            }

            counter -= 1;
        }

    private static void ProcessTierMONEYValues(ClientTripTypeContractChargeValueList tripTypeContractChargeValueList, int counter)
    {
        foreach (var chargeTierMONEYValue in tripTypeContractChargeValueList.ChargeTierMONEYValues)
        {
            chargeTierMONEYValue.ChargeTierMONEYValueFKID = counter;
        }
    }

    private static void ProcessENUMValues(ClientTripTypeContractChargeValueList tripTypeContractChargeValueList, int counter)
    {
        foreach (var chargeENUMValue in tripTypeContractChargeValueList.ChargeENUMValues)
        {
            chargeENUMValue.ChargeENUMValueFKID = counter;
        }
    }

    private static void ProcessDECIMALValues(ClientTripTypeContractChargeValueList tripTypeContractChargeValueList, int counter)
    {
        foreach (var chargeDECIMALValue in tripTypeContractChargeValueList.ChargeDECIMALValues)
        {
            chargeDECIMALValue.ChargeDECIMALValueFKID = counter;
        }
    }

    private static void ProcessBITValues(ClientTripTypeContractChargeValueList tripTypeContractChargeValueList, int counter)
    {
        foreach (var chargeBITValue in tripTypeContractChargeValueList.ChargeBITValues)
        {
            chargeBITValue.ChargeBITValueFKID = counter;
        }
    }
\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot, this is a very good attempt, but i don't find it generic or it don't help me remove some redundant code from my app \$\endgroup\$
    – MegaMind
    Jan 3, 2012 at 15:47
0
\$\begingroup\$

Tried something using Observables and Reactive extensions.

"using System.Reactive.Linq;" (For the operator)

"using System.Reactive.Concurrency" (For the scheduler, to speed up observing process)

Two options..

Code Snippet # 1 (Managing only loops, any one of the condition will be performed)

int counter = -1;

newClientTripTypeContracts.ToObservable<ClientTripTypeContractChargeValue>().Subscribe<ClientTripTypeContractChargeValue>(clientTripTypeContractItem =>
    {
        clientTripTypeContractItem.ClientTripTypeContractChargeValueListID = counter;

    if (clientTripTypeContractItem.ChargeBITValues.Count > 0)
        clientTripTypeContractItem.ChargeBITValues.ToObservable<ChargeBIT>().SubscribeOn(Scheduler.Immediate).ObserveOn(Scheduler.TaskPool).Subscribe(innerItem => innerItem.ChargeBITValueFKID = counter);
    else if (clientTripTypeContractItem.ChargeDECIMALValues.Count > 0)
        clientTripTypeContractItem.ChargeDECIMALValues.ToObservable<ChargeDECIMAL>().SubscribeOn(Scheduler.Immediate).ObserveOn(Scheduler.TaskPool).Subscribe(innerItem => innerItem.ChargeDECIMALValueFKID = counter);
    else if (clientTripTypeContractItem.ChargeENUMValues.Count > 0)
        clientTripTypeContractItem.ChargeENUMValues.ToObservable<ChargeENUM>().SubscribeOn(Scheduler.Immediate).ObserveOn(Scheduler.TaskPool).Subscribe(innerItem => innerItem.ChargeENUMValueFKID = counter);
    else if (clientTripTypeContractItem.ChargeTierMONEYValues.Count > 0)
        clientTripTypeContractItem.ChargeTierMONEYValues.ToObservable<ChargeTierMONEY>().SubscribeOn(Scheduler.Immediate).ObserveOn(Scheduler.TaskPool).Subscribe(innerItem => innerItem.ChargeTierMONEYValueFKID = counter);

    counter -= 1;
});

Code Snippet # 2 (Managing loops and each list is called if any items found, FKID is assigned value of counter)

    int counter = -1;
newClientTripTypeContracts.ToObservable<ClientTripTypeContractChargeValue>()
    .Subscribe<ClientTripTypeContractChargeValue>(clientTripTypeContractItem =>
    {
        clientTripTypeContractItem.ClientTripTypeContractChargeValueListID = counter;
        clientTripTypeContractItem.ChargeBITValues.ToObservable<ChargeBIT>().SubscribeOn(Scheduler.Immediate).ObserveOn(Scheduler.TaskPool).Subscribe(innerItem => innerItem.ChargeBITValueFKID = counter);
        clientTripTypeContractItem.ChargeDECIMALValues.ToObservable<ChargeDECIMAL>().SubscribeOn(Scheduler.Immediate).ObserveOn(Scheduler.TaskPool).Subscribe(innerItem => innerItem.ChargeDECIMALValueFKID = counter);
        clientTripTypeContractItem.ChargeENUMValues.ToObservable<ChargeENUM>().SubscribeOn(Scheduler.Immediate).ObserveOn(Scheduler.TaskPool).Subscribe(innerItem => innerItem.ChargeENUMValueFKID = counter);
        clientTripTypeContractItem.ChargeTierMONEYValues.ToObservable<ChargeTierMONEY>().SubscribeOn(Scheduler.Immediate).ObserveOn(Scheduler.TaskPool).Subscribe(innerItem => innerItem.ChargeTierMONEYValueFKID = counter);
        counter -= 1;
    });

Hope it would help. Not tested fully but simple enough to manage. P.S: Use LinqPad to dump values and debug the composition

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.