Best Practices for try-catch blocks inside the Get part of an indexer [closed]

Question

Closed. This question is opinion-based. It is not currently accepting answers.

Want to improve this question? Because this question may lead to opinionated discussion, debate, and answers, it has been closed. You may edit the question if you feel you can improve it so that it requires answers that include facts and citations or a detailed explanation of the proposed solution. If edited, the question will be reviewed and might be reopened.

Closed 4 months ago.

Improve this question

I've been creating a basic version of a LinkedList class mainly for the sake of an assignment (though also to gain a better understanding of it all). I've gotten to the stage where I'm writing error handling into everything where necessary, and the part that I've been struggling most with is the linked list's indexer property, but specifically the "Get" part of it (only showing the indexer itself because I think that's the only relevant section):

/// <summary>
/// Indexer that allows you to either retrieve or set the data of a node 
/// within the linked list
/// </summary>
/// <param name="index">the index of the node to be retrieved or changed</param>
/// <returns></returns>
/// <exception cref="Exception">Throws exception if faulty index or value given.</exception>
public T this[int index]
{
    get
    {
        try
        {
            if (index < 0) { throw new Exception("Index cannot be less than 0"); }
            else if (head is null) { throw new Exception("This list has no items in it."); }
            else if (index >= Count) { throw new Exception("Index overflow (too high)."); }
            else if (index == 0) { return head.Data; }
            else if (index == count - 1) { return tail.Data; }
            else
            {
                CustomLinkedNode<T> current = head;
                for (int currNum = 0; currNum < index; currNum++)
                {
                    current = current.Next;
                }
                return current.Data;
            }
        } catch(Exception e)
        {
            Console.WriteLine(e.Message);
        }
    }
    set
    {
        try
        {
            if (index < 0) { throw new Exception("Index cannot be less than 0"); }
            else if (head is null) { throw new Exception("This list has no items in it."); }
            else if (index >= Count) { throw new Exception("Index overflow (too high)."); }
            else
            {
                CustomLinkedNode<T> current = head;
                for (int currNum = 0; currNum < index; currNum++)
                {
                    current = current.Next;
                }
                current.Data = value;
            }
        } catch(Exception e)
        {
            Console.WriteLine(e.Message);
        }
    }
}

The reason as to why I show the 'Set' part of the indexer is because as you can see, both of them generally use the same type of logic when it comes to catching and handling errors. The weird part? The Get block gets a CSO161 "Not all code paths return a value" error whereas the Set block does not. What would be the best way to alter the Get block code to resolve this error?

"What would be the best way to alter the Get block code to resolve this error" - removing the catch block... (It's not weird that this fails to compile - what do you expect to be returned when you catch the exception?) — Jon Skeet
– Jon Skeet, Commented Mar 18 at 17:58
The standard thing to do here is for list indexers just to throw exceptions for invalid indices and let the caller handle their mistake. See for instance List<T>.Item[Int32] which throws an ArgumentOutOfRangeException for invalid indices. Also, if you want to avoid code duplication in the setters & getters you could extract an internal method returning the CustomLinkedNode<T> for the specified index like so: dotnetfiddle.net/sk0c3Z. — dbc
– dbc, Commented Mar 18 at 18:32
See also Framework design guidelines: Property Design: AVOID throwing exceptions from property getters... Notice that this rule does not apply to indexers, where we do expect exceptions as a result of validating the arguments. — dbc
– dbc, Commented Mar 18 at 18:38
@Neptunium-Eater - sure, it's possible. You could for instance specify some default value of type T to return from your CustomLinkedList<T> when an invalid index is passed in. But is it wise? I'd argue not. At such a low level, any errors inside your list indexer are likely due to some bug in CustomLinkedList<T> itself or its immediate callers and so by swallowing the exception you would only be covering up the problem. — dbc
– dbc, Commented Mar 18 at 20:30
Rule of thumb: allow most exceptions to bubble up. Only swallow what you can safely handle — Charlieface
– Charlieface, Commented Mar 18 at 21:36

Olivier Jacot-Descombes · Accepted Answer · 2026-03-18 18:18:42Z

2

The getter must return a value or, alternatively, it can throw an exception. So, an easy way to handle it is to not catch exceptions and let them occur, or you can catch them and instead trow a more appropriate one.

You can also show a message box and then re-throw the exception like this:

try
{
   ...
} catch(Exception e)
{
    Console.WriteLine(e.Message);
    throw; // re-throw this exception.
}

edited Mar 18 at 18:18

answered Mar 18 at 18:10

Olivier Jacot-Descombes

115k14 gold badges149 silver badges202 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

JonasH · Accepted Answer · 2026-03-19 09:52:24Z

The problem is that if an error is thrown you will end up in the catch statement, and this just prints to the console, it neither returns a value or throws an exception, that is why you get the compiler error.

Throwing an exception and immediately catching it just to write the error to the console is not very useful. I would advice to just remove the try catch. Let the caller decide how to deal with the error. Logging errors where they are thrown is generally an antipattern since it can lead to log spam. It is usually a better idea to make sure the exception has sufficiently detailed information to fix the problem and let the caller decide if and how it should be logged.

I would also recommend throwing a meaningful type of exception. In this case this would be IndexOutOfRangeException. Another 'trick' is to merge the lower and upper bounds check by casting to an unsigned type, if ((uint)index >= Count). When casting to an unsigned type any negative values will become larger than int.MaxValue, so this is both slightly faster, and less code. Likewise, if head is null then count should be zero, so that check can also be removed.

The if (index == 0) condition can also be removed, since the else with the loop will do the right thing if index is zero. You could also remove the if (index == count - 1) check for the same reason. That will have some performance impact, but linked lists have rather terrible performance characteristics anyway, so I would prioritize simpler code.

So I would write the getter something like this:

if ((uint)index >= Count) throw new IndexOutOfRangeException($"Index {index} is out of range. Count is {Count}");
var current = head;
for (;index > 0; index--)
{
    current = current.Next;
}
return current.Data;

I would also consider just not providing an indexer for a linked list. If there is an indexer I would expect it to be fast, i.e. constant time. I would instead implement the IReadOnlyCollection<T> interface, create an iterator, and use ElementAt if I needed access to a specific element. Ofc, this will probably not be allowed if this is an assignment.

Collectives™ on Stack Overflow

Best Practices for try-catch blocks inside the Get part of an indexer [closed]

2 Answers 2

Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Related