Laden...

Designfrage: ArgumentOutOfRangeException oder IndexOutOfRangeException

Erstellt von sheitman vor 16 Jahren Letzter Beitrag vor 16 Jahren 3.826 Views
S
sheitman Themenstarter:in
1.047 Beiträge seit 2005
vor 16 Jahren
Designfrage: ArgumentOutOfRangeException oder IndexOutOfRangeException

Guten Morgen.

Gegeben ist folgende Klasse:

class A {
    private int[] iArray;

    public A (int[] iArray) {
        if(iArray == null) {
            throw new ArgumentNullException("iArray");
        }
        this.iArray = iArray;
    }

    public int GetInt_1(int index) {
        if(index < 0 || index >= iArray.Length) {
            throw new ArgumentOutOfRangeException("index");
        }
        return iArray[index];
    }

    public int GetInt_2(int index) {
        //wenn index < 0 || index >= iArray.Length wird eine
        //IndexOutOfRangeException vom System geworfen
        return iArray[index];
    }
}

Nun stellt sich mir die Frage welches Design (GetInt_1 oder GetInt_2) ist "besser" bzw. wendet ihr an, vielleicht auch warum.
Ich hab bisher immer nach methodik GetInt_1 gearbeitet, da ich Argumente eh meist überprüfe und dann bei nem Indexer das einfach mitmache.
Chef meinet aber im Grunde ist es überflüßige Arbeit das ja das System eh ne Exception schmeißt.

Einzigsten Vorteil den ich gerade sehe ist, das ArgumentOutOfRangeException von ArgumentException erbt, d.h. das Exceptionhandling ist etwas anders da ich auch ArgumentNullExceptions darüber mit fangen kann, während IndexOutOfRangeException ja eine SystemException ist.

Was meint ihr dazu?

Grüße,
Sven

Gelöschter Account
vor 16 Jahren
class A {
    private int[] iArray;

    public A (int[] iArray) {
        if(iArray == null) {
            throw new ArgumentNullException("iArray");
        }
        this.iArray = iArray;
    }

    public int Integer1[index]
 {

get{
        if(index < 0 || index >= iArray.Length) {
            throw new IndexOutOfRangeException (....);
        }
        return iArray[index];
}
    }

    
} 

so würde ich es lösen

49.485 Beiträge seit 2005
vor 16 Jahren

Hallo sheitman,

also sauberer wäre schon eine ArgumentOutOfRangeException, damit für den Verwender der Klasse klar ist, dass er "schuld" ist, weil er falsche Argumente übergeben hat. Bei einer IndexOutOfRangeException muss er ja erstmal ermitteln, ob die an seinen falschen Argumenten lag oder an einem Programmierfehler der benutzen Klasse - wobei ich bei einer IndexOutOfRangeException erstmal letztes vermuten würde.

Außerdem ist es für das Verständnis des Codes besser, wenn die Argumente am Anfang geprüft werden und man - quasi durch Code als Dokumentation - gleich sieht, was zulässig ist und was nicht.

Natürlich hat dein Chef recht, dass es weniger Arbeit ist, eine IndexOutOfRangeException werfen zu lassen, aber die Qualität des Codes/der Klasse ist dann - wenn auch nur geringfügig - schlechter.

herbivore

S
8.746 Beiträge seit 2005
vor 16 Jahren

Dein Chef hat völlig recht. Entspricht den Microsoft-Guidelines zu Exception-Verwendung in C#. Eine IndexOutOfRangeException in ArgumentOutOfRange zu verwandeln ist definitiv falsch. Auch das widerspricht den Guidelines:

      if(index < 0 || index >= iArray.Length) {
            throw new IndexOutOfRangeException (....);

Zumindest wenn anschließend direkt ein Zugriff erfolgt der zu der gleichen Exception führt. Anders kann man argumentieren, wenn der Index nur für eine spätere Verwendung gespeichert wird. Ist aber hier nicht der Fall.

Die Geschichte mit der ArgumentNullException ist hingegen ok.

40 Beiträge seit 2007
vor 16 Jahren

Das verwundert mich jetzt ein wenig. Wenn ich die Klasse verwende kann ich doch mit einer IndexOutOfRangeEx nichts anfangen, ich weiss ja nicht wie die Klasse implementiert wurde? Schliesslich greif ich ja nicht mit einem Indexer drauf zu...

Eine ArgumentOutOfRangeEx ist da doch viel deutlicher und weist mich auf meinen Fehler hin.

Sehe ich das jetzt falsch?

49.485 Beiträge seit 2005
vor 16 Jahren

Hallo svenson,

Entspricht den Microsoft-Guidelines zu Exception-Verwendung in C#.

glaube ich nicht und entspricht auch nicht dem im Framework implementieren. So wirft z.B. List<T>.Insert(int index, T item) natürlich eine ArgumentOutOfRangeException wenn der Index nicht stimmt.

herbivore

S
8.746 Beiträge seit 2005
vor 16 Jahren

Eine List<T> ist ja auch kein Array:

IndexOutOfRangeException Class

The exception that is thrown when an attempt is made to access an element of an array with an index that is outside the bounds of the array.

glaube ich nicht

* Do not create methods that throw NullReferenceException or IndexOutOfRangeException.

Error Raising and Handling Guidelines

Generell gilt: Nur dann Exceptions werfen, wenn ein fachlicher Mehrwert entsteht. Ansonsten schwillt der Code mit Exceptionhandling an und konterkariert die Eleganz und den Sinn von Exceptions.

Anders sieht das z.B. in Java mit checked Exceptions aus. Der Verzicht auf diese in C# wurde u.a. mit diesem Stil von Exception-Behandlung begründet.

S
sheitman Themenstarter:in
1.047 Beiträge seit 2005
vor 16 Jahren

Eine List<T> ist ja auch kein Array.

glaube ich nicht

* Do not create methods that throw NullReferenceException or IndexOutOfRangeException.


>

Naja aber es hat intern ein Array, genau wie meine Klasse oben.
Meine Klasse hat auch einen Indexer der aber nichts darüber sagt das ich ein Array intern benutze.

Also wie ich das so sehe hat herbivore (mal wieder 😉) schon die besten Argumente angeführt:

also sauberer wäre schon eine ArgumentOutOfRangeException, damit für den Verwender der Klasse klar ist, dass er "schuld" ist, weil er falsche Argumente übergeben hat. Bei einer IndexOutOfRangeException muss er ja erstmal ermitteln, ob die an seinen falschen Argumenten lag oder an einem Programmierfehler der benutzen Klasse - wobei ich bei einer IndexOutOfRangeException erstmal letztes vermuten würde.

Außerdem ist es für das Verständnis des Codes besser, wenn die Argumente am Anfang geprüft werden und man - quasi durch Code als Dokumentation - gleich sieht, was zulässig ist und was nicht.

Werd mir jetzt mal die Guidelines anschaun^^

Nachtrag:
im Grunde stehts ja auc in de Guidelines bzw interpertiere ich sie so das ich auf den Konsens von herbivore komme:

Do not create methods that throw NullReferenceException or IndexOutOfRangeException.

wollte ich ja nie werfen, die Frage war vorher zu prüfen auf das sie nciht auftreten und ne passende Exception werfen oder einfach zulassen, Jacklena's Weg war ja doppelt gemoppelt der eh nicht zur Diskussion stand und auch diesem Guide wiederspricht.

Viel interessanter find ich

All code paths that result in an exception should provide a method to check for success without throwing an exception. For example, to avoid a FileNotFoundException you can call File.Exists. This might not always be possible, but the goal is that under normal execution no exceptions should be thrown.

und

Throw an ArgumentException or create an exception derived from this class if invalid parameters are passed or detected.

was herbivores Post auch untermauert. ^^

D.h. also ich könnte wenn der Index nicht paßt ne Exception werfen oder aber einen Standardwert zurück geben sodaß keien Exception aufritt, man müßte das halt in der Hilfe vermerken.
Häufig haben solche Methoden ja auch ein boolschen Rückgabewert der angibt ob die Fkt ausgeführt werden konnte. Und für Fälle wo man sowas nicht machen kann gibts ja immernoch die ArgumentExceptions^^

S
8.746 Beiträge seit 2005
vor 16 Jahren

Ist allerdings richtig, diese Klasse ist auch kein Array. Ich würde allerdings - wenn schon denn schon - auf eine Prüfung verzichten und stattdessen über ein Exception-Handling arbeiten. Spart Performance.

Der Satz bezgl. NullReference- und Index meint MS wohl wirklich restriktiv:

Thrown by the runtime only when an array is indexed improperly.
49.485 Beiträge seit 2005
vor 16 Jahren

Hallo svenson,

Eine IndexOutOfRangeException in ArgumentOutOfRange zu verwandeln ist definitiv falsch.

* Do not create methods that throw NullReferenceException or IndexOutOfRangeException.

Da widerspricht die Guideline aber genau deiner Aussage und stützt meine. Es wäre also gerade richtig - wie ich ja auch gesagt habe -, eine ArgumentOutOfRangeException zu werfen.

Generell gilt: Nur dann Exceptions werfen, wenn ein fachlicher Mehrwert entsteht. Ansonsten schwillt der Code mit Exceptionhandling an und konterkariert die Eleganz und den Sinn von Exceptions.

Den Mehrwert habe ich ja erläutert.

herbivore

S
sheitman Themenstarter:in
1.047 Beiträge seit 2005
vor 16 Jahren

Ist allerdings richtig, diese Klasse ist auch kein Array. Ich würde allerdings - wenn schon denn schon - auf eine Prüfung verzichten und stattdessen über ein Exception-Handling arbeiten. Spart Performance.

Glaub das Thema wäre einen witeren Thread wert.
Mein Chef mag keine Exceptions so gern, da ja doch rel. teuer und man soll Exception afaik auch nicht als Kontrollstrukturen a la if else benutzen... von daher wäre wohl

bool GetInt(int index, out int i)

für sowas wohl sogar noch eleganter, oder?

S
8.746 Beiträge seit 2005
vor 16 Jahren

Mein Chef mag keine Exceptions so gern, da ja doch rel. teuer und man soll Exception afaik auch nicht als Kontrollstrukturen a la if else benutzen...

Da hat er recht, deswegen gilt ja auch:

All code paths that result in an exception should provide a method to check for success without throwing an exception. For example, to avoid a FileNotFoundException you can call File.Exists. This might not always be possible, but the goal is that under normal execution no exceptions should be thrown.

Gutes Beispiel ist Parse()/TryParse. Aber das ist eine fachliche Frage (was ist normal?) und keine grundsätzliche. Wer _grundsätzlich _Exceptions nicht mag, ist m.E. auf dem Holzdampfer.

In deinem Falle ist aber der fehlerhafter Index-Zugriff eine Ausnahme und nicht die Regel. Deswegen die explizite Prüfung in der Regel unnötig. In einem sauberen Programm nie. Also sind Exceptions angezeigt und _verbessern _die Performance.

49.485 Beiträge seit 2005
vor 16 Jahren

Hallo sheitman,

All code paths that result in an exception should provide a method to check for success without throwing an exception.

du brauchst deine Methode nicht umzustellen, weil der Check ja dadurch möglich ist, dass man weiß wie lang/groß die Liste ist und man daher als Aufrufer vorher prüfen/wisssen kann, ob der Index innerhalb des Bereiches liegt.

Mit sind Methode, die boolsche Werte statt Exceptions liefern eher suspekt. Hier würde ich die jedenfalls nicht verwenden.

herbivore

S
8.746 Beiträge seit 2005
vor 16 Jahren

Hab grad mal in den .NET-Code geschaut. Interessanterweise macht .NET die Prüfung auch explizit. Gründe fallen mir dafür aber nicht ein...

49.485 Beiträge seit 2005
vor 16 Jahren

Hallo svenson,

Hab grad mal in den .NET-Code geschaut. Interessanterweise macht .NET die Prüfung auch explizit.

Das hatte ich ja oben (indirekt) schon geschrieben.

Gründe fallen mir dafür aber nicht ein...

Naja, zum einen steht es im Guide, dass man es so machen soll. Und inhaltlich habe ich ja schon die Gründe genannt, warum man es machen sollte.

herbivore

S
sheitman Themenstarter:in
1.047 Beiträge seit 2005
vor 16 Jahren

Zum Thema TryXY hab ich noch was nettes gefunden

Consider the TryParse pattern for members that may throw exceptions in common scenarios to avoid performance problems related to exceptions.

To implement The TryParse pattern, you provide two different methods for performing an operation that can throw exceptions in common scenarios. The first method, X, does the operation and throws the exception when appropriate. The second method, TryX, does not throw the exception, but instead returns a Boolean value indicating success or failure. Any data returned by a successful call to TryX is returned using an out (ByRef in Visual Basic) parameter. The Parse and TryParse methods are examples of this pattern.
Do provide an exception-throwing member for each member using the TryParse pattern.

It is almost always not correct design to provide only the TryX method because it requires understanding out parameters. Also, the performance impact of exceptions is not an issue for most common scenarios; you should provide methods that are easy to use in most common scenarios. Exceptions and Performance

In deinem Falle ist aber der fehlerhafter Index-Zugriff eine Ausnahme und nicht die Regel. Deswegen die explizite Prüfung in der Regel unnötig. In einem sauberen Programm nie. Also sind Exceptions angezeigt und verbessern die Performance.

Verstehe worauf du hinaus willst.

Mit sind Methode, die boolsche Werte statt Exceptions liefern eher suspekt. Hier würde ich die jedenfalls nicht verwenden.

Naja beim Dictionary find ich sie gut, da sie mir letzlich einen Aufruf mehr spart und für den Fall gemacht ist das ich ein Item haben möchte und vorher aber prüfe ob es auch da ist.
Paßt im dem Fall mit dem Index sicher nicht, aber bei Key -> Value Situationen schon wie ich find.

S
8.746 Beiträge seit 2005
vor 16 Jahren

Hab grad mal in den .NET-Code geschaut. Interessanterweise macht .NET die Prüfung auch explizit.
Das hatte ich ja oben (indirekt) schon geschrieben.

Kann da keinen Bezug zu diesem Problem erkennen. Warum macht MS haufenweise explizite Prüfung, die in der Regel nie notwendig sind? Der fehlerhafte Index-Zugriff kann doch kaum als "normal condition" eine Collection-Klasse bezeichnet werden.

BTW: Reflector-Bug gefunden. Wenn man folgendem Code traut, dann würde IndexOutOfRange kommen:



private T[] _items;

public T this[int index]
{
    get
    {
        if (index >= this._size)
        {
            ThrowHelper.ThrowArgumentOutOfRangeException();
        }
        return this._items[index];
    }
    set
    {
        if (index >= this._size)
        {
            ThrowHelper.ThrowArgumentOutOfRangeException();
        }
        this._items[index] = value;
        this._version++;
    }
}

Tatsächlich wird aber index nach uint gecastet, was der Reflector erkennen kann, aber nicht tut.

49.485 Beiträge seit 2005
vor 16 Jahren

Hallo svenson,

ich denke, Parameter (explizit) zu prüfen ist schon aus Anlehnung an Programming by Contract sinnvoll.

herbivore

S
sheitman Themenstarter:in
1.047 Beiträge seit 2005
vor 16 Jahren

Tatsächlich wird aber index nach uint gecastet, was der Reflector erkennen kann, aber nicht tut.

Dann schick dem Lutz Roeder mal ne Email^^

S
8.746 Beiträge seit 2005
vor 16 Jahren

Dann schick dem Lutz Roeder mal ne Email^^

Schon passiert...

S
8.746 Beiträge seit 2005
vor 16 Jahren

ich denke, Parameter (explizit) zu prüfen ist schon aus Anlehnung an Programming by Contract sinnvoll.

Ja, super Sache, aber in purem C# leider so unvollständig implementierbar, dass ich lieber komplett darauf verzichte. Mit Spec# sieht das schon anders aus. Richtig Sinn macht das m.E. erst, wenn man eine statische Analyse a la Boogie drüber laufen lassen kann.

49.485 Beiträge seit 2005
vor 16 Jahren

Hallo svenson,

ja, das ist wohl wahr. Allerdings kann man den "was garantiert mir der Aufrufer"-Teil doch ziemlich vollständig prüfen. Und um den Teil geht es hier ja.

herbivore

S
8.746 Beiträge seit 2005
vor 16 Jahren

Es geht mir doch darum, dass diese Prüfung sowieso implizit stattfindet. Es geht nicht um das "ob", sondern um das "wie":

if (indexok)
   arrayzugriff // hier nochmal
else
 throw ArgumentException

vs

try
  arrayzugriff
catch IndexOutOfRangeException
  throw ArgumentException

Beide Implementierungen sind semantisch äquivalent, letztere aber schneller (normal condition). So hätte ich auch den Collection-Code erwartet... Sofern die Implentierungen gegen ein Interface gecodet wäre, könnte man das Vorgehen verstehen, ist aber nicht der Fall.

49.485 Beiträge seit 2005
vor 16 Jahren

Hallo svenson,

dafür gibt es m.E. mehrere Gründe. Eine Parameterüberprüfung sollte am Anfang stehen. Wird die Verarbeitung ohne die Prüfung begonnen, kann der daraus resultierende Fehler ja u.U. erst auftreten, wenn ein Teil der Verarbeitung erfolgt ist, sprich das Objekt könnte dadurch in einen inkonsistenten Zustand gekommen sein, der dann erst wieder (aufwändig) korrigiert werden muss. Außerdem könnte es sein, dass ein falscher Parameter gar nicht (sofort) zu einer Exception führt, und der falsche Parameter dadurch gar nicht gemeldet wird. Außerdem eine Prüfung am Anfang natürlich auch noch den von mir schon genannten Dokumentationseffekt. Also schon gute Gründe quasi doppelt zu prüfen.

herbivore

S
8.746 Beiträge seit 2005
vor 16 Jahren

Das mag im Einzelfall als Begründung taugen, aber nicht im konkreten Falle des obigen Indexers.

49.485 Beiträge seit 2005
vor 16 Jahren

Hallo svenson,

ich würde sagen, meine Begründung greift gerade im allgemeinen Fall. Und aus Einheitlichkeitsgründen macht man es auch in diesem Einzelfall - wo es anders gehen würde - ebenso.

herbivore

S
8.746 Beiträge seit 2005
vor 16 Jahren

Ich erwarte halt ein bißchen mehr, als nur nach Schema F vorzugehen. Ein wenig Blick auf die Performance - gerade bei so kritischen Baustellen - wäre m.E. angezeigt. Für mich sieht das eher so aus, als hätten C-Coder auf C# umgeschult.

49.485 Beiträge seit 2005
vor 16 Jahren

Hallo svenson,

Einheitlichkeit ist nun mal ein Wert. Wenn du das als Schema F abtust, wirst du der Sache m.E. nicht gerecht. Natürlich hätte man in Abwägung mit anderen Gründen (hier: Performance) zu einer anderen Entscheidung kommen können, aber dadurch wird die getroffene Entscheidung trotzdem nicht fragwürdig. Wie eine (Design-)Entscheidung ausfällt, hängt immer davon ab, wie man die einzelnen Aspekte gewichtet. Da du diese Gewichtung nicht kennst, finde ich es anmaßend, über die Fähigkeiten der Programmierer negativ zu spekulieren.

herbivore

S
8.746 Beiträge seit 2005
vor 16 Jahren

Wie eine (Design-)Entscheidung ausfällt, hängt immer davon ab, wie man die einzelnen Aspekte gewichtet.

Das steht außer Frage. Aber hier hätte ich anders abgewogen.

Aber schon lustig, wenn man sich mal das Framework hinsichtlich des Exceptionshandlings anschaut. Man findet alles.

Typ bla = obj as Typ;
if (bla != null)

Genauso wie harte Casts und fangen der InvalidCastException.

Auch schön war:

catch (Exception ex)
{
    if ( (ex == A) || (ex == b) || ....))
       throw;
}

als Gegenpart zu

 catch (ArgumentException)
    {
    }
    catch (NotSupportedException)
    {
    }
    catch (SecurityException)
    {
    }
    catch (IOException)
    {
    }
    catch (UnauthorizedAccessException)
    {
    }
    return flag;

Deutlich merkt man auch den Unterschied von 1.1 und 2.0-Code (direktes Werfen vs. ThrowHelper).

Mein Gesamteindruck: Das BCL-Team hat einen deutlich anderen Stil drauf, als der Rest des Frameworks. In der BCL werden fast immer die Eingangsparameter geprüft. ArgumentNullExceptions, etc.! In System.Xml praktisch überhaupt nicht mehr der Fall. Da kann man praktisch überall eine NullReferenceException provozieren.

Die Stringenz des BCL-Teams hat aber auch ihre Schattenseiten: Es wird blind dokumentiert. Gutes Beispiel: Array.GetValue(long). Es fehlt selbst in der aktuellen Doku die IndexOutOfRangeException (die wird eben nicht zu Beginn geworfen, wie die anderen). ArgumentOutOfRange ist falsch dokumentiert (valid heisst in diesem Falle: Kleiner als int32). Das ist wohlgemerkt eine 1.1-Funktion.