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.
