I'm running a loop of type object which is otherOrders, then inside another for loop changing the object value and copying in the same list but everytime I change the new value the previous one gets override. Please suggest me how to not change previous value
List<OtherOrder> otherOrders = calling some function and getting value;
foreach (var otherOrder in otherOrders.ToList())
{
var docs = OtherOrdersManager.GetOtherOrderDeliveryDocumentAttachments(otherOrder.Id);
if (docs.Count > 1)
{
for (var i = 0; i < docs.Count; i )
{
otherOrder.ResultDate = docs[i].DocumentCreationText;
otherOrders.Add(otherOrder);
}
}
I want to change the result date value without getting override the previous one and save it in same or another list will also work. If you have asnwer regarding instance and clone please suggest with code. Pls help
CodePudding user response：
First problem, try to be more careful when using cycle variables because naming it in the same way as the fullList is incredibly confusing, expecially for us that can't just hover over the variable to see it's type, the other problem I see is that you change the .ResultDate of the actual obj but then you add it again in the main list, what you should do is remove the otherOrders.Add(otherOrder); because you are already updating the value inside the main list, if you want instead to create a new List and populate it from the start just change the code to this:
List<OtherOrder> otherOrders = calling some function and getting value;
var newList = new List<otherOrder>();
foreach (var order in otherOrders.ToList())
{
var docs = OtherOrdersManager.GetOtherOrderDeliveryDocumentAttachments(order.Id);
if (docs.Count > 1)
{
for (var i = 0; i < docs.Count; i )
{
order.ResultDate = docs[i].DocumentCreationText;
newList.Add(order);
}
}
One thing that's good to know it that sometimes when you do something like this newList.Add(otherOrder); it just creates a pointer to the value of order (this means that when the value of order changes the value inside newList changes as well, what we should do instead is to manually add every param inside the new list, this assure you that the values are not pointed at the main lst we used previously:
newList.Add(new otherOrder()
{
param1 = order.param1,
param2 = order.param2
// go on until you set up all the params
});
So try either one of these approaches and tell me if it works for you, but the most important one is the name accountability, so remember that before everything else!
CodePudding user response：
This is the 2 lines that shows the obvious issue:
otherOrder.ResultDate = docs[i].DocumentCreationText;
otherOrders.Add(otherOrder);
Here otherOrder is already an item in the otherOrders list. It sounds like at this point what you really wanted to do was create a clone or copy of the otherOrder, change the ResultDate and add that to the list.
It is not obvious from this logic alone why you would be cloning the whole order with only the date being changed, it might be valid in your business context but it is not clear from the current code. Future editors might mistake this logic so I would recommend changing variable names to be more descriptive and include a comment of some kind in there.
There are a number of ways to implement cloning of objects, from an OO point of view and if you want to ensure that all instances of your logic that want to clone your object to do it the same way, then this is a behaviour of that type of object and you should encapsulate that logic within the definition of that type.
Originally in c# you could implement the IClonable interface, however that is strongly discouraged:
Note: If you need a cloning mechanism, define your own
Clone, orCopymethodology, and ensure that you document clearly whether it is a deep or shallow copy. An appropriate pattern is:public <type> Copy();
How you actually implement that is up to you, but it should involve the instantiation of a new instance of your OtherOrder type and the initialization of the properties and/or fields that you want to be copied into the new object.
Though not the most efficient from a performance perspective, you can use serialization to copy an object with minimal code, this is a very simple solution using Json.NET
public OtherOrder Copy()
{
var cereal = JsonConvert.SerializeObject(value);
return (OtherOrder)JsonConvert.DeserializeObject(cereal, typeof(OtherOrder));
}
You could also define this Copy logic in a factory or static utility class, or even convert it into a generic or extension method. But it is up to you to define and control how you want to clone/copy your object instances.
Including the use of this method, I have added verbose pseudo code comments into your logic to highlight the intent of each action:
// 1. Get a list of `OtherOrders`
List<OtherOrder> otherOrders = calling some function and getting value;
// 2. Create a copy of the list, and then iterate the items
// NOTE: This is a standard convention that allows us to
// iterate the original list using `foreach` whilst making changes to it
foreach (var order in otherOrders.ToList())
{
// 3. for each order, get the attachments
var docs = OtherOrdersManager.GetOtherOrderDeliveryDocumentAttachments(otherOrder.Id);
// 4. If there is more than 1 attachment, then we need to duplicate the order
// The external system can only process individual documents
if (docs.Count > 1)
{
// for each attachment, copy the original order, but transfer the unique information
// into the clone. Then put this new object into the original list.
foreach (var attachment in docs)
{
var orderClone = otherOrder.Copy();
orderClone.ResultDate = attachment.DocumentCreationText;
otherOrders.Add(orderClone);
}
}
}