Is It Bad Practice to Update an Object in a Static Method?

cnet

Still learning .net and OOP and have a basic question I hope. This is an asp.net mvc app.

Looking at the code below, I am most concerned with the line in CheckShippingVendor that says ValidateDelivery(shipmentItem). Is it okay to pass in an object as a parameter and have the private static method update that item.

When I did a resharper Extract Method refactoring this is what it did for me.

Class is instantiated and call the CheckShippingVendor method: 

var packageStatus = _inventoryService.CheckShippingVendor(shipmentId, shipmentFile);

Here is the rest of the code: 

public PackageStatus CheckShippingVendor(string shipmentId, mvFile shipmentFile)
{
        var packageStatus = new PackageStatus();
        using (var shipmentItem = shipmentFile.Read(shipmentId))
        {
                ValidateDelivery(shipmentItem);
                shipmentFile.Write(shipmentId, shipmentItem);
        }
        return packageStatus;
}

private static void ValidateDelivery(mvItem shipmentItem)
{
        int shipped;
        var shippedResult = Int32.TryParse(shipmentItem[1].ToString(), out shipped);
        if (!shippedResult)
            return;

        int delivered;
        var deliveredResult = Int32.TryParse(shipmentItem[57].ToString(), out delivered);
        if (!deliveredResult)
            return;

        shipmentItem.Message = "NO INFORMATION AT THIS TIME";
}

Best Answer

Note: this question may be better served in codereview.stackexchange.com

In general, I try to keep static methods side-effect free.

In your particular example, I would change it as follows:

public PackageStatus CheckShippingVendor(string shipmentId, mvFile shipmentFile)
{
    var packageStatus = new PackageStatus();
    using (var shipmentItem = shipmentFile.Read(shipmentId))
    {
        if(!ValidateDelivery(shipmentItem))
            shipmentItem.Message = "NO INFORMATION AT THIS TIME";

        shipmentFile.Write(shipmentId, shipmentItem);
    }
    return packageStatus;
}

private static bool IsDeliveryValid(mvItem shipmentItem)
{
    int shipped;
    var shippedResult = Int32.TryParse(shipmentItem[1].ToString(), out shipped);
    if (!shippedResult)
        return false;

    int delivered;
    return Int32.TryParse(shipmentItem[57].ToString(), out delivered);
}

This does two things:

  1. It removes the side-effect from your static method
  2. It reduces your validation method to a single responsibility: giving you a yes/no answer to whether or not a delivery is valid.

That said, I would iterate on the CheckShippingVendor method some more:

  • The name gives no indication that there are side effects.
  • It always returns an empty PackageStatus, rendering the return value useless.
  • It appears to have several responsibilities which could be broken into separate helper methods.
Related Topic