Forse avrete capito che in questi giorni sto facendo un po' di refactoring su un'applicazione di un cliente e scavando nel codice se ne scoprono delle belle. Oggi con il mio fidato debugger mi sono imbattuto in questa curiosa implementazione di una proprietà:
public NameValueCollection Values
{
get
{
if (_multiValue == null && _stringValue != null)
{
_stringValue = Preprocess(_stringValue);
_multiValue = new HttpValueCollection();
if ((_stringValue.IndexOf('&') >= 0) || (_stringValue.IndexOf('=') >= 0))
{
_multiValue.FillFromString(_stringValue);
}
else
{
_multiValue.Add(null, _stringValue);
}
_stringValue = null;
}
return _multiValue;
}
}
In prima battuta, a parte il fatto che 10 righe di codice per un get sono un po' troppe, che la leggibilità potrebbe essere migliorata e che ci sono un po' troppi if, non ho notato nulla di grave. Ma guardando poche righe più sotto ho trovato queste altre due proprietà:
public string StringValue
{
get { return _stringValue; }
set { _stringValue = value; }
}
internal HttpValueCollection MultiValue
{
get { return _multiValue; }
set { _multiValue = value; }
}
La pericolosità sta nel fatto che il get della proprietà Values modifica lo stato (pubblico) dell'oggetto. Questo aveva strani effetti sul codice che utilizzava questa classe, perché a seconda dell'ordine in cui venivano lette le proprietà il comportamento cambiava.
Il problema è correlato a quello dell'altro
post in cui un metodo Add in alcuni rimuoveva anche un oggetto dalla collezione. E' importante che un metodo o una proprietà faccia una sola cosa, possibilmente quella per cui è stata scritta.
In questo caso, oltre a questo problema di molteplici compiti, abbiamo anche un get che modifica lo stato dell'oggetto e questo causa non pochi problemi. La lettura di una proprietà non deve mai modificare lo stato dell'oggetto, per quello esistono i metodi.