CODEWITHSUNDEEP
c#parametersreturn-typeside-effects
mainvoid
mainvoid

Reputation: 357

Side effects on collection items or return a new collection?

Let's say I have a WriteItem class that looks like this:

public class WriteItem
{
    public string Name { get; set; }
    public object Value { get; set; } 
    public int ResultCode { get; set; }
    public string ErrorMessage { get; set;}
}

I need to process each item and set its ResultCode and ErrorMessage properties and I though about defining a method similar to this:

public void ProcessItems(WriteItemCollection items)
{
    foreach(var item in items)
    {
        // Process each item and set its result.
    }
}

The processing of each item is done by another class.

Is this the best way to do it?

Or is it better to have the method return a collection of a custom Result class?

Upvotes: 1

Views: 113

Answers (4)

Ewan
Ewan

Reputation: 1285

It is better to return a new results class.

why?

As others have said you are modifying the collection and its not really clear. But for me this is not the main reason. You can have processes which modify objects.

For me its because you have had to add extra properties to your WriteItem object in order to support the processor. This in effect creates a strong coupling between the model and a processor which should not exist.

Consider you have another method ProcessItems_ForSomeOtherPurpose(List<WriteItem> items) do you expand your ResultCode int to have more meaningful values? do you add another property ResultCode_ForSomeOtherPurpose? what if you need to process the same item mutiple times with multiple processors?

I would give your Model an Id. Then you can log multiple processes against it

eg.

item 1 - loaded

item 1 - picking failed!

item 1 - picked

item 1 - delivered

Upvotes: 0

Steven Lemmens
Steven Lemmens

Reputation: 630

I think it all comes down to readability.

When you call ProcessItems, is it obvious that the collection has changed? If you call the method like this:

var items = GetItemsFromSomewhere();
ProcessItems(items);

versus calling it like this:

var items = GetItemsFromSomewhere();
items = ProcessItems(items);

or simply changing your methodname:

var items = GetItemsFromSomewhere();
items = UpdateItemStatuses(items);

In the end there is no right answer to this question in my book. You should do what feels right for your application. And consider: what if another developer was looking at this piece of code? Can he surmise what is happening here, or would he have to dive into the ProcessItems-function to get the gist of the application.

Upvotes: 0

Heinzi
Heinzi

Reputation: 172200

Both options have their advantages and disadvantages. Both are "fine" in the sense that there is nothing wrong with them and they are commonly used in C#.

Option 1 has the big advantage of being simple and easy. You can even keep a reference to a WriteItem instance and check its status after processing.

Option 2 has a clearer separation of concerns: In Option 1, you need to add comments to your WriteItem class to define which are "input" and which are "output" properties. Option 2 does not need that. In addition, Option 2 allows you to make WriteItem and ProcessingResult immutable, which is a nice property.

Option 2 is also more extensible: If you want to process something else than WriteItems (with the same return options), you can define a class

class ProcessingResult<T>
{
    public T Item { get; set; }
    public int ResultCode { get; set; }
    public string ErrorMessage { get; set; }
}

and use it as ProcessingResult<WriteItem> as well as ProcessingResult<SomeOtherItem>.

Upvotes: 1

Mathieu A.
Mathieu A.

Reputation: 103

What you wrote will work. You can modify the object properties without having side effects while iterating in the collection.

I wouldn't return a new collection unless you need to keep a copy of the original collection untouched.

Upvotes: 0

Related Questions