Alternativen zu geschachtelten Using Statements?

*Q* 1

Hallo zusammen,

ich habe gerade eine Methode geschrieben mit ziemlich vielen Ressourcen (IDisposable).

Irgendwie stört mich das mit den ganzen geschachtelten using clauses.
Geht das irgendwie besser/schöner?

string[] files = Directory.GetFiles(Directory.GetCurrentDirectory(), "mandel*.zip");
string outputDir = Directory.GetCurrentDirectory() + string.Format(@"\output\{0:yyyy_MM_dd_HH_mm_ss}", DateTime.Now);
foreach (string fileName in files)
{
    try
    {
        Console.WriteLine("Processing file {0}...", fileName);
        using (ZipFile zipFile = ZipFile.Read(fileName))
        {
            foreach (ZipEntry entry in zipFile.Entries)
            {
                using (Stream zipStream = entry.OpenReader())
                {
                    using (BinaryReader reader = new BinaryReader(zipStream))
                    {
                        try
                        {
                            ConvertFile(reader, outputDir);
                            Console.WriteLine("{0} was successfully converted", entry.FileName);
                        }
                        catch (Exception e)
                        {
                            Console.WriteLine("Error: Invalid Format in file {0}", fileName);
                            Console.WriteLine("Details: " + e);
                        }
                    }
                }
            }
        }
    }
    catch
    {
        Console.WriteLine("Error: Failed to open file {0}", fileName);
    }
}

GPC

Was schon mal geht ist mehrere direkt aufeinanderfolgende using-Statements zu gruppieren:

using (Stream zipStream = entry.OpenReader())
using (BinaryReader reader = new BinaryReader(zipStream)) {
  //...
}

Zeus

Vielleicht so?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace ConsoleApplication1
{
    class Test : IDisposable
    {
        public void Dispose()
        {
            System.Console.WriteLine("Bye");
        }
    }

    class Program
    {
        static void Main(string[] args)
        {
            using(Test a = new Test())
            using(Test b = new Test())
            {
            }
        }
    }
}

Dravere

Zudem ist dein zweites using -Statement gar nicht nötig. BinaryReader wrappt nur den Stream . Wenn du somit Dispose auf dem reader aufrufst, macht der nichts anderes, als Dispose vom übergebenen Stream Objekt aufzurufen.

Du benötigst auch das zipStream Objekt gar nicht. Bau dein Code daher am besten gleich etwas um:

using(var reader = new BinaryReader(entry.OpenReader()))
{
  // ...
}

Mit der Verwendung von var , wird dies übrigens auch noch einiges kürzer

Je nach dem kann übrigens auch der äussere Dispose Aufruf auf das ZipFile Objekt alle Streams schliessen. Das liest du aber am besten in der entsprechenden Doku nach.

Zudem gibt es noch den Tipp, dass du Funktionen erstellen kannst. Das dürfte auch die Wartbarkeit für den Code erhöhen. Ich denke da an Funktionen wie:

void ConvertZipFile(string filename)
{
  using (var zipFile = ZipFile.Read(fileName))
  {
    foreach (var entry in zipFile.Entries)
    {
      ConvertZipEntry(entry);
    }
  }
}

void ConvertZipEntry(ZipEntry entry)
{
  using (var reader = new BinaryReader(entry.OpenReader()))
  {
    // ...
  }
}

Das ergibt deutlich kürzere und einfachere Funktionen mit weniger tiefer Verschachtelung und damit auch leserlicher.

Grüssli

*Q* 1

Danke für eure Hilfe!

Das mit using-Statements gruppieren ist eine gute Sache. Werd ich in Zukunft benutzen.

Dravere schrieb:

Du benötigst auch das zipStream Objekt gar nicht. Bau dein Code daher am besten gleich etwas um:
using(var reader = new BinaryReader(entry.OpenReader()))
{
  // ...
}

wenn entry.OpenReader funktioniert, aber new BinaryReader z.B. eine OutOfMemoryException wirft, dann wird der reader nicht geschlossen, oder?
Ist zwar ziemlich unwahrscheinlich, aber gerade im c++ forum wird ja immer viel wert auf exceptionsicherheit gelegt

Das mit Aufteilung in Methoden ist mir natürlich auch klar. Ich wollte nur eben mal quick&dirty nen kurzes programm schreiben. Als nächstes wäre ich jetzt erstmal mit Refactor->Extract Method darauf losgegangen.

Ich hab übrigens noch folgendes rausgefunden:
C# hat mit stackalloc sogar die Möglichkeit, etwas auf dem stack anzulegen. Ich finds nur sehr schade, dass das nicht gleichzeitig auch zu einem automatischen Dispose führt.

Dravere

Q schrieb:

wenn entry.OpenReader funktioniert, aber new BinaryReader z.B. eine OutOfMemoryException wirft, dann wird der reader nicht geschlossen, oder?
Ist zwar ziemlich unwahrscheinlich, aber gerade im c++ forum wird ja immer viel wert auf exceptionsicherheit gelegt

Musst du gar kein Smilie hinsetzen, der Einwand ist durchaus gerechtfertig. Obwohl ich auch der Meinung bin, dass die Wahrscheinlichkeit eher klein ist. Aber das ist natürlich keine Rechtfertigung für den Fehler.

Q schrieb:

Ich hab übrigens noch folgendes rausgefunden:
C# hat mit stackalloc sogar die Möglichkeit, etwas auf dem stack anzulegen. Ich finds nur sehr schade, dass das nicht gleichzeitig auch zu einem automatischen Dispose führt.

stackalloc ist auch nur für unsafe Code gedacht. Bei stackalloc geht es nur um zwei Dinge: Interoperabilität und Performance. Du kannst im übrigen stackalloc auch gar nicht mit Referenztypen verwenden.
Schieb daher diese Erkenntnis lieber ziemlich weit nach hinten in deinem Kopf. stackalloc ist etwas, was du kaum je einmal benutzen wirst.

Grüssli

hustbaer

z.T. stackalloc:
Wenn der Compiler sieht dass ein Objekt beim verlassen eines Scopes automatisch "unreachable" wird, dann legt er es doch selbst auf den Stack, oder nicht?
Also auch "class" Typen.

Zumindest hab ich das so in Erinnerung.

Könnte nur sein dass ich wo gelesen habe dass das eine mögliche Optimierung wäre, die aber nicht umgesetzt wurde.

Dravere

hustbaer schrieb:

z.T. stackalloc:
Wenn der Compiler sieht dass ein Objekt beim verlassen eines Scopes automatisch "unreachable" wird, dann legt er es doch selbst auf den Stack, oder nicht?
Also auch "class" Typen.

Nein. Wäre mir zumindest neu und habe es zur Sicherheit auch schnell in einem einfachen Programm getestet, macht mein MS C# 4.0 Kompiler nicht.

Könnte es sein, dass du das damit verwechselst, dass die Referenz auf das Objekt selber natürlich auf dem Stack angelegt wird. Und die Value-Typen werden natürlich trotz new auf dem Stack angelegt.

Edit: Gut, was natürlich sein könnte ist, dass der JIT dies dann so umbaut, aber daran zweifle ich etwas.

Grüssli

hustbaer

Nene, ich meine schon das Objekt selbst, und nicht die Referenz.

Hab auch grad ein wenig gegoogelt. Das ganze nennt sich "escape analysis", und .NET macht es anscheinend (noch?) nicht. Schade.

Dravere

hustbaer schrieb:

(noch?)

Hast du etwas darüber gelesen, dass dies überhaupt in Erwägung gezogen wird für C#?

Soweit mir bekannt ist, gibt es ja dieses Feature in der JVM. Allerdings frage ich mich, ob man das dort nicht eingeführt hat, um eben das Fehlen von eigenen Value Typen auszugleichen. Bzw. dass man in C# deswegen dies gar nicht als nötig oder irgendwie wichtig sieht.

Grüssli

hustbaer

Nö, hab dazu nix gelesen.

Wäre aber eine schöne Optimierung.
Und öffnet den Weg zu weiteren schönen Optimierungen, z.B. Lock-Elision.

mMn. müsste das auch halbwegs einfach zu machen sein.

Würde mich daher nicht wundern wenn das noch irgendwann kommt.

Wäre auch für C++ eine feine Sache. Dummerweise erlaubt es der aktuelle Standard AFAIK nicht, da dort glaube ich vorgeschrieben ist dass, wenn die Klasse keinen eigenen definiert, der globale operator new aufgerufen wird. Da man den selbst definieren darf, muss man den Aufruf als beobachtbar werten, und der Compiler darf den Aufruf nicht mehr einfach so wegoptimieren.
Wobei es in C++ wohl weniger wichtig ist, da man dort mit weniger new auskommt.

I heard you like IDisposable. So i put some IDisposables in a IDisposable

using System;
using System.Collections.Generic;

namespace CleanUp
{
    class Program
    {
        static void Main(string[] args)
        {
            A a = new A();
            B b = new B();
            B b2 = new B();
            A a2 = new A();

            using (Disposables dispo = new Disposables { a, b, b2, a2 })
            {
                a.A_func();
                b.B_func();
                b2.B_func();
                a2.A_func();
            }

            Console.ReadKey(true);
        }
    }

    class Disposables : IDisposable, IEnumerable<IDisposable>
    {
        List<IDisposable> disposables = new List<IDisposable>();

        public void Dispose()
        {
            disposables.Reverse();
            foreach (var disposable in disposables)
                disposable.Dispose();
        }

        public void Add(IDisposable disposable)
        {
            disposables.Add(disposable);

        }
        public IEnumerator<IDisposable> GetEnumerator()
        {
            return disposables.GetEnumerator();
        }

        System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator()
        {
            return disposables.GetEnumerator();
        }
    }

    class A : IDisposable
    {
        public void A_func()
        {
            Console.WriteLine("A_func");
        }
        public void Dispose()
        {
            Console.WriteLine("Dispose A");
        }
    }

    class B : IDisposable
    {
        public void B_func()
        {
            Console.WriteLine("B_func");
        }
        public void Dispose()
        {
            Console.WriteLine("Dispose B");
        }
    }
}

Firefighter

@µ: Wir nennen es : *mit Tiefer Stimme sag*The Ultimate Disposer!

*Q* 1

Schöne Idee, aber leider hats auch das Problem das es nicht mehr exceptionsicher ist (zumindest so wie du es hier verwendet hast).

A a = new A(); 
B b = new B(); 
B b2 = new B(); 
A a2 = new A(); 

using (Disposables dispo = new Disposables { a, b, b2, a2 }) 
{ 
       a.A_func(); 
       b.B_func(); 
       b2.B_func(); 
       a2.A_func(); 
}

wenn new B()
oder new A()
schmeißen, werden die schon vorher erzeugten objekte nicht freigegeben.

Evtl. wäre es so besser:

using(Disposer disposer = new Disposer())
{
   A a = new A();
   disposer.Add(a);
   B b = new B();
   disposer.Add(b);
   //...
}

Wobei das Add auch wieder schmeißen könnte

Aber ich benutze jetzt einfach die Variante bei der die inneren usings die direkt auf ein äußeres folgen nicht weiter eingerückt werden.

Q schrieb:

Schöne Idee, aber leider hats auch das Problem das es nicht mehr exceptionsicher ist (zumindest so wie du es hier verwendet hast).

Exceptions die in einem Konstruktor geworfen werden, übergehen sowieso den finally-Block.

hustbaer

Ich würde das wenn dann eher so machen:

class Disposer : IDisposable, IEnumerable<IDisposable>
	{
		List<IDisposable> m_disposables = new List<IDisposable>();

		public void Dispose()
		{
			if (m_disposables.Count > 0)
			{
				// don't use .Reverse() so that double-disposing won't lead to strange behavior
				for (int i = m_disposables.Count - 1; i >= 0; i--)
				{
					// Dispose() shouldn't throw, but it might, and we need to dispose everything that was added
					// so just catch and ignore all exceptions
					try
					{
						m_disposables[i].Dispose();
					}
					catch
					{
					}
				}
			}
		}

		public T Add<T>(T disposable)
		{
			Add(disposable as IDisposable);
			return disposable;
		}

		public IDisposable Add(IDisposable disposable)
		{
			try
			{
				if (disposable != null)
					m_disposables.Add(disposable);

				return disposable;
			}
			catch
			{
				disposable.Dispose();
				throw;
			}
		}

		public IEnumerator<IDisposable> GetEnumerator()
		{
			return m_disposables.GetEnumerator();
		}

		System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator()
		{
			return m_disposables.GetEnumerator();
		}
	}

Und verwenden ala

using (Disposer dispo = new Disposer())
				{
					A a = dispo.Add(new A());
					B b = dispo.Add(new B());
					B b2 = dispo.Add(new B());
					A a2 = dispo.Add(new A());

					// ... do stuff ...
				}

wollt ihr euch gegenseitig veräppeln? das kann doch unmöglich ernst gemeint sein

Dravere

µ schrieb:

Exceptions die in einem Konstruktor geworfen werden, übergehen sowieso den finally-Block.

Wie bitte? Oder versteh ich dich hier grad nicht richtig? Hast du dazu Quellen oder kannst das etwas genauer erläutern?

@hustbaer,
Ehm ... ja. Hast du sowas jemals schon eingesetzt?

Das Ignorieren der Exception find ich aber mal eine ganz schlechte Idee. Du könntest zumindest ein rethrow der letzten Exception machen, nachdem alle Dispose Methoden aufgerufen wurden. Oder vielleicht sogar eine neue Exception werfen, welche auf die Exception hinweist: DiposeException?

Grüssli

Nächster Versuch. Hatte gestern nach Qs Einwand noch folgendes im Sinn. Die Exceptions gehen nicht verloren.
Sieht jemand Schwachstellen?

using System;
using System.Collections.Generic;

namespace CleanUp
{
    class Program
    {
        static void Main(string[] args)
        {
            try
            {
                using (Disposables dispo = new Disposables())
                {
                    A a = dispo.Create<A>();
                    B b = dispo.Create<B>("hurr");
                    C c = dispo.Create<C>(a);

                    a.A_func();
                    b.B_func();
                }
            }
            catch(Exception ex)
            {
                Console.WriteLine(ex.InnerException.Message);
            }

            Console.ReadKey(true);
        }
    }

    class Disposables : IDisposable
    {
        List<IDisposable> disposables = new List<IDisposable>();

        public T Create<T>() where T : IDisposable, new()
        {
            T t = new T();
            disposables.Add(t);
            return t;
        }

        public T Create<T>(params object[] args) where T : IDisposable
        {
            T t = (T)Activator.CreateInstance(typeof(T), args);
            disposables.Add(t);
            return t;
        }

        public void Dispose()
        {
            disposables.Reverse();
            foreach (var disposable in disposables)
                disposable.Dispose();
        }
    }

    class A : IDisposable
    {
        public void A_func()
        {
            Console.WriteLine("A_func");
        }
        public void Dispose()
        {
            Console.WriteLine("Dispose A");
        }
    }

    class B : IDisposable
    {
        string s;
        public B(string s)
        {
            this.s = s;
        }
        public void B_func()
        {
            Console.WriteLine("B_func: {0}", s);
        }
        public void Dispose()
        {
            Console.WriteLine("Dispose B");
        }
    }

    class C : IDisposable
    {
        public C(A a)
        {
            throw new Exception("exception in C");
        }
        public void Dispose()
        {
            Console.WriteLine("Dispose C");
        }
    }
}

@Dravere:

class Q : IDisposable
{
	public Q()
	{
		throw new Exception();
	}

	public void Dispose()
	{
		Console.WriteLine("Dispose Q");
	}
}

Dann wird bei
using (Q q = new Q()) { }
Dispose nicht aufgerufen. Klar, das Objekt wurde ja nicht vollständig konstruiert.
Hat aber nicht direkt etwas mit dem Einwand des Ops zu tun. Schieben wir es auf die Übermüdung

Wobei ich an hustbaers Vorschlag auch kein großes Problem sehe. Das try-catch in Add() hat irgendwie keinen Sinn, warum sollte dort eine Exception auftreten können?
Und eine kummulative Exception in Dispose bauen, dann sollte alles sauber sein.