Still learning .net and OOP and have a basic question I hope. This is an asp.net
ID: 641992 • Letter: S
Question
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";
}
Explanation / Answer
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.