2012-04-23 20 views
9

Ho un codice che ha un sacco di duplicati. Il problema deriva dal fatto che ho a che fare con i tipi nidificati IDisposable. Oggi ho qualcosa che assomiglia a:In che modo un codice refactor può essere coinvolto negli usi annidati?

public void UpdateFromXml(Guid innerId, XDocument someXml) 
{ 
    using (var a = SomeFactory.GetA(_uri)) 
    using (var b = a.GetB(_id)) 
    using (var c = b.GetC(innerId)) 
    { 
     var cWrapper = new SomeWrapper(c); 
     cWrapper.Update(someXml); 
    } 
} 

public bool GetSomeValueById(Guid innerId) 
{ 
    using (var a = SomeFactory.GetA(_uri)) 
    using (var b = a.GetB(_id)) 
    using (var c = b.GetC(innerId)) 
    { 
     return c.GetSomeValue(); 
    } 
} 

L'intero annidato using blocco è lo stesso per ciascuno di questi metodi (due sono mostrati, ma ci sono una decina di loro). L'unica cosa che è diversa è ciò che accade quando arrivi al livello interno dei blocchi using.

Un modo pensavo sarebbe quella di fare qualcosa sulla falsariga di:

public void UpdateFromXml(Guid innerId, XDocument someXml) 
{ 
    ActOnC(innerId, c => 
    { 
     var cWrapper = new SomeWrapper(c); 
     cWrapper.Update(someXml); 
    }); 
} 

public bool GetSomeValueById(Guid innerId) 
{ 
    var result = null; 

    ActOnC(innerId, c => { result = c.GetSomeValue(); }); 

    return result; 
} 

private void ActOnC(Guid innerId, Action<TheCType> action) 
{ 
    using (var a = SomeFactory.GetA(_uri)) 
    using (var b = a.GetB(_id)) 
    using (var c = b.GetC(innerId)) 
    { 
     action(c); 
    }   
} 

Questo funziona, è solo tipo di goffo per analizzare (come un essere umano). Qualcuno ha altri suggerimenti su come si potrebbe ridurre la duplicazione del codice attorno ai blocchi nidificati using in questo modo? Se non fossero IDisposable allora probabilmente si creerebbe un metodo per restituire i risultati di b.GetC(innerId) ... ma non è questo il caso.

+3

+1 io non vedo nulla goffo nella soluzione. È un po 'non ortodosso essere più funzionale di procedurale, ma lo considererei un pro, non un con – mfeingold

+1

Penso che la tua implementazione sia soddisfacente, ma forse preferirai alcune delle alternative fornite di seguito. Se ti ritrovi a dover ricorrere a un sacco di articoli usa e getta dovresti guardare a ridisegnare le cose in modo da non finire in questa situazione. – Thomas

risposta

1

mi piace la risposta fornita da BFree come un inizio, ma mi piacerebbe fare alcune modifiche.

//Give it a better name; this isn't designed to be a general purpose class 
public class MyCompositeDisposable : IDisposable 
{ 
    public MyCompositeDisposable (string uri, int id, int innerid) 
    { 
     A = SomeFactory.GetA(uri); 
     B = A.GetB(id); 
     C = B.GetC(innerId); 
    } 

    //You can make A & B private if appropriate; 
    //not sure if all three or just C should be exposed publicly. 
    //Class names are made up; you'll need to fix. 
    //They should also probably be given more meaningful names. 
    public ClassA A{get;private set;} 
    public ClassB B{get;private set;} 
    public ClassC C{get;private set;} 

    public void Dispose() 
    { 
     A.Dispose(); 
     B.Dispose(); 
     C.Dispose(); 
    } 
} 

Dopo aver fatto che si può fare qualcosa di simile:

public bool GetSomeValueById(Guid innerId) 
{ 
    using(MyCompositeDisposable d = new MyCompositeDisposable(_uri, _id, innerId)) 
    { 
     return d.C.GetSomeValue(); 
    } 
} 

Nota che il MyCompositeDisposable sarà probabilmente bisogno di avere try/finally blocchi nel costruttore e Dispose metodi in modo che gli errori nella creazione/distruzione correttamente assicurarsi che nulla finisca per essere eliminato.

+0

L'idea di avvolgere tutto in una classe come questa è perfetta per le mie esigenze e offre il giusto equilibrio tra deduplicazione del codice e flessibilità per tutti i miei casi, inoltre aiuta anche a separare le preoccupazioni un po '. Questo aveva più o meno la migliore di tutte le risposte. Grazie. – ckittel

+0

Questo ha lo stesso difetto della risposta di BFree - un'eccezione durante la costruzione di C lascerà A e B non disposti. –

+1

@DavidB Avevo già una nota alla fine della risposta che un tale controllo degli errori è necessario, ma che non è stato incluso nella risposta qui. Se è necessario nel caso dell'OP sa che ha bisogno di aggiungerlo. – Servy

0

Se i vostri tipi Dispoable dispongono correttamente di tutti i membri usa e getta, è necessario solo uno che utilizza la dichiarazione.

Ad esempio, questo:

public bool GetSomeValueById(Guid innerId) 
{ 
    using (var a = SomeFactory.GetA(_uri)) 
    using (var b = a.GetB(_id)) 
    using (var c = b.GetC(innerId)) 
    { 
     return c.GetSomeValue(); 
    } 
} 

potrebbe diventare questo se A avesse membri b del tipo e c, e un disposto di B e C nel suo metodo dispose:

public bool GetSomeValueById(Guid innerId) 
{ 
    using (var a = SomeFactory.GetA(_uri)) 
    { 
     return a.GetSomeValue(); 
    } 
} 

class A : IDisposable 
{ 
    private a; 
    private b; 

    public A (B b, C c) 
    { 
    this.b = b; this.c = c; 
    } 

    public void Dispose() 
    { 
    Dispose(true); 
    } 

    protected void Dispose(bool disposing) 
    { 
    if (disposing) 
    { 
     b.Dispose(); 
     c.Dispose(); 
    } 
    } 
} 

Si sarebbe devi modificare la tua fabbrica per iniettare bec in a, comunque.

+2

Si deve fare attenzione quando gli oggetti dispongono di oggetti dati loro da un'altra classe. Cosa succede se più di un'istanza si basa su quell'oggetto? Lo smaltimento dovrebbe essere generalmente responsabilità della classe proprietaria, e in questo caso 'A' non possiede' b' e 'c'. – Thomas

+0

@Thomas punto eccellente. In genere, si avranno anche parametri boolean ctor che indicano se A possiede b e c. – jrummell

1

Nel quadro Rx c'è una classe chiamata CompositeDisposablehttp://msdn.microsoft.com/en-us/library/system.reactive.disposables.compositedisposable%28v=vs.103%29.aspx

non dovrebbe essere troppo difficile per rotolare il proprio (anche se molto ridotta versione):

public class CompositeDisposable : IDisposable 
{ 
    private IDisposable[] _disposables; 

    public CompositeDisposable(params IDisposable[] disposables) 
    { 
     _disposables = disposables; 
    } 

    public void Dispose() 
    { 
     if(_disposables == null) 
     { 
      return; 
     } 

     foreach(var disposable in _disposables) 
     { 
      disposable.Dispose(); 
     } 
    } 
} 

Allora questo sembra un po 'più pulito:

public void UpdateFromXml(Guid innerId, XDocument someXml) 
{ 
    var a = SomeFactory.GetA(_uri); 
    var b = a.GetB(_id); 
    var c = b.GetC(innerId); 
    using(new CompositeDisposable(a,b,c)) 
    { 
     var cWrapper = new SomeWrapper(c); 
     cWrapper.Update(someXml); 
    } 
} 
+2

cosa succede se si verifica un'eccezione durante b.GetC - Non penso che a e b siano disposti correttamente quando ciò accade. –

1

È sempre possibile creare un contesto più ampio in cui gestire gli oggetti da creare/eliminare. Quindi scrivi un metodo per creare un contesto più ampio ...

public class DisposeChain<T> : IDisposable where T : IDisposable 
{ 
    public T Item { get; private set; } 
    private IDisposable _innerChain; 

    public DisposeChain(T theItem) 
    { 
     this.Item = theItem; 
     _innerChain = null; 
    } 

    public DisposeChain(T theItem, IDisposable inner) 
    { 
     this.Item = theItem; 
     _innerChain = inner; 
    } 

    public DisposeChain<U> Next<U>(Func<T, U> getNext) where U : IDisposable 
    { 
     try 
     { 
      U nextItem = getNext(this.Item); 
      DisposeChain<U> result = new DisposeChain<U>(nextItem, this); 
      return result; 
     } 
     catch //an exception occurred - abort construction and dispose everything! 
     { 
      this.Dispose() 
      throw; 
     } 
    } 

    public void Dispose() 
    { 
     Item.Dispose(); 
     if (_innerChain != null) 
     { 
      _innerChain.Dispose(); 
     } 
    } 
} 

quindi utilizzarlo:

public DisposeChain<DataContext> GetCDisposeChain() 
    { 
     var a = new DisposeChain<XmlWriter>(XmlWriter.Create((Stream)null)); 
     var b = a.Next(aItem => new SqlConnection()); 
     var c = b.Next(bItem => new DataContext("")); 

     return c; 
    } 

    public void Test() 
    { 
     using (var cDisposer = GetCDisposeChain()) 
     { 
      var c = cDisposer.Item; 
      //do stuff with c; 
     } 
    }