È una cattiva pratica aggiornare un oggetto in un metodo statico?

Nota: questa domanda potrebbe essere meglio servita in codereview.stackexchange.com

In generale, cerco di mantenere i metodi statici senza effetti collaterali.

Nel tuo particolare esempio, lo cambierei come segue:

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);
}

Questo fa due cose:

1. Rimuove l'effetto collaterale dal metodo statico
2. Riduce il tuo metodo di convalida ad una singola responsabilità: ti dà una risposta sì / no alla consegna o meno di una consegna.

Detto questo, vorrei ripetere il metodo di CheckShippingVendor ancora:

• Il nome non indica che ci sono effetti collaterali.
• Restituisce sempre un PackageStatus vuoto, rendendo il valore di ritorno inutile.
• Sembra che ci siano diverse responsabilità che potrebbero essere suddivise in metodi di supporto separati.

La modifica di un oggetto passato come parametro a un metodo statico è una tecnica perfettamente valida. È valido, perché non stai mantenendo nessuna modifica di stato all'esterno dell'oggetto che si sta modificando.

Se il tuo metodo statico stava cambiando alcuni stati statici nella tua classe statica, sarebbe una storia diversa. Questo accordo è ampiamente considerato come "non verificabile".

Tuttavia, il nome del metodo statico potrebbe essere più chiaro. Non c'è alcuna indicazione nel nome del metodo in cui l'oggetto che si sta passando viene modificato. Cambiare il nome del metodo in qualcosa come UpdateDeliveryStatus() o cambiare il metodo in modo che restituisca un valore che rappresenta lo stato di consegna senza modificare l'oggetto passato.

È accettabile che un metodo statico modifichi una classe passata come parametro. Dopotutto, questo è ciò che i metodi di estensione fanno.

Dove le persone si mettono nei guai sta tentando di memorizzare lo stato in variabili statiche e di modificarlo e utilizzarlo attraverso metodi statici.

Nel tuo caso specifico, il metodo ValidateDelivery sta facendo tre cose: convalidare la spedizione, convalidare la consegna e assegnare lo stato di consegna.

Prenderò in considerazione l'ipotesi di inserire il codice di convalida sull'oggetto ( mvItem ) stesso. Se ciò non può essere fatto, considera di scrivere un metodo di estensione per farlo. Nella mia mente ha molto più senso chiedere all'oggetto stesso se si tratta di una consegna valida o meno.

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);
}

Questo, per me, separa molto meglio le tue preoccupazioni.

Non c'è nulla di male nell'aggiornare gli oggetti nei metodi statici, i metodi statici appartengono alla classe, non all'oggetto. È necessario un metodo statico quando è necessario fornire funzionalità comuni a tutti gli oggetti o all'ottimizzazione della memoria, perché se si crea un nuovo oggetto per accedere solo a un singolo metodo, si spreca di memoria e si aumenta il sovraccarico nella gestione della memoria.