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";
}
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:
- It removes the side-effect from your static method
- 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.
1
Modifying an object passed in as a parameter to a static method is a perfectly valid technique. It is valid, because you are not holding any state changes outside the object being modified.
If your static method were changing some static state in your static class, that would be a different story. That arrangement is widely regarded as “untestable.”
Your static method name could be clearer, however. There is no indication in the name of the method that the object being passed in is being modified. Either change the method name to something like UpdateDeliveryStatus(), or change the method so that it returns some value that represents the delivery state without modifying the passed-in object.
1
It is acceptable for a static method to modify a class passed in as a parameter. After all, this is what extension methods do.
Where people get into trouble is attempting to store state in static variables and modifying and using that state through static methods.
In your specific case, the ValidateDelivery method is doing three things – validating shipment, validating delivery and assigning delivery status.
I would consider placing the validation code on the object (mvItem) itself. If this can’t be done, then consider writing an extension method to do so. In my mind it makes a lot more sense to ask the item itself whether it’s a valid delivery or not.
private static void ValidateDelivery(mvItem shipmentItem)
{
if (shipmentItem.IsShipped() && shipmentItem.IsDelivered())
{
shipmentItem.Message = "NO INFORMATION AT THIS TIME";
}
}
public static bool IsShipped(this mvItem shipmentItem)
{
int shipped;
return Int32.TryParse(shipmentItem[1].ToString(), out shipped);
}
public static bool IsDelivered(this mvItem shipmentItem)
{
int delivered;
return Int32.TryParse(shipmentItem[57].ToString(), out delivered);
}
This, to me, separates out your concerns much more nicely.
There’s no harm in updating objects in static methods, Static methods belong to class, not to object. You need static method when you need to provide common functionality to all objects or memory optimization, because if you create a new object for accessing only a single method, it’s memory wasting and increasing overhead in memory management.
1