C# – Need help reviewing the code [closed]

Posted on

Problem

Here is a method I have:

private void ProcessExpiryDate(out DateTime cashDate, out DateTime expiryDate, out DateTime expiryDateSecond)
{
// Lots of logic here, that's why I created this method
}

This private method is called several times in other public methods.
And every time a call is needed, I need to do the following codes:

DateTime cashDate;
DateTime expiryDate;
DateTime expiryDateSecond;
ProcessExpiryDate(out cashDate, out expiryDate, out expiryDateSecond);
existingObject.CashDate = cashDate;
existingObject.ExpiryDate = expiryDate;
existingObject.ExpiryDateSecond = expiryDateSecond;

Is there any better suggestion to make the lines shorter during the call ?
These 7 lines I need to write in every call looks ugly.

Solution

It’s preferable to not allow out parameters in your code.

Methods should be immutable as far as possible and not change the state of incoming data.

I would:

  1. Return an object that encapsulates cashDate, expiryDate, expiryDateSecond (ex: ProcessedExpiryDate object)
  2. Use a method such as SetDates as Maxim had suggested

This will allow you to turn your method call into:
SetDates(existingObject, ProcessExpiryDate())

You’re not the only one who noticed this problem. With C#7 they give us a better way to do it, with inline out parameters:

ProcessExpiryDate(out DateTime cashDate, 
                  out DateTime expiryDate, 
                  out DateTime expiryDateSecond);

But if you’re not ready to leap into version 7, you could just pass the object in and let the method set the dates. Or better yet, define an interface.

interface IExpirable
{
    DateTime CashDate { set; }
    DateTime ExpiryDate { set; }
    DateTime ExpiryDateSecond { set; }
}

class ExistingObject : IExpirable
{
    //Define as you did before
}

private void ApplyExpiryDate(IExpirable o)
{
    o.CashDate = moe; 
    o.ExpiryDate = larry;
    o.ExpiryDateSecond = curly;
    //No need to return the modified object, because it was passed by reference
}

void MainCodeExample(ExistingObject existingObject)
{
    ApplyExpiryDate(existingObject);
}

Notes:

  1. In keeping with common standards I switched from camelCase to PascalCase for the property names.

  2. I changed “Process” to “Apply” in the method name, which I think is more likely to be understand as a method which changes the object that is passed in.

Why not to turn your ProcessExpiryDate method to this?

private void SetDates(SomeObject existingObject)
{
    // calculate dates

    existingObject.CashDate = cashDate;
    existingObject.ExpiryDate = expiryDate;
    existingObject.ExpiryDateSecond = expiryDateSecond;
}

And all your code will be turned into

SetDates(existingObject);

Leave a Reply

Your email address will not be published. Required fields are marked *