8
\$\begingroup\$

There at least 2 patterns of using Stack.Pop

  1. private Stack<T> availableResources;
    ...
    if (availableResources.Count > 0) 
    {
      resource = availableResources.Pop();
      ...
    }
    
  2. private Stack<T> availableResources;
    ...
    try 
    {
      resource = availableResources.Pop();
      ...
    }
    catch (InvalidOperationException e)
    {
      ...
    }
    

I'd like to ask people, which of 2 is preferable? If possible, argument your answer.

\$\endgroup\$
  • \$\begingroup\$ Save your exceptions for exceptional cases you don't know how to handle. Patterns 2 is good for wrapping a method that can fail for various non-related or unknown reasons (DownloadRemoteFile(url)). Or for python. \$\endgroup\$ – avip Nov 15 '12 at 22:12
16
\$\begingroup\$

Program defensively and use option 1.

Since the Stack class provides the ability to check whether it contains anything before trying to Pop a value from it, you should do the check and avoid using the exception for flow control.

If you want to avoid doing the if check throughout your application, you could create a TryPop extension.

public static class StackExtensions
{
    public static bool TryPop<T>(this Stack<T> stack, out T value)
    {
        if (stack.Count > 0)
        {
            value = stack.Pop();
            return true;
        }

        value = default(T);
        return false;
    }
}

It could be used in the following way:

T resource;
if (availableResources.TryPop(out resource)) 
{
    // use `resource` for something
}

It's worth noting that this approach is only suitable if the Stack is not accessed concurrently! - If it is, and you are using .NET 4.0 onwards then you should use ConcurrentStack, otherwise create your own concurrent stack which encapsulates a Stack and manages access to it with locks.

\$\endgroup\$
  • 3
    \$\begingroup\$ Be careful when implementing your own concurrent stack. For example, checking Count before Pop() is not thread-safe, even if each of those methods is. You would need a thread-safe TryPop() and no Pop(), like .Net 4.0 ConcurrentStack does. \$\endgroup\$ – svick Nov 15 '12 at 21:23
1
\$\begingroup\$

First we should check, is there concurrent access to stack, or not. If it is, the first pattern doesn't handle an exception ('cause between the check and popping itself there can be an additional popping from the other thread), so consider using either ConcurrentStack<> or double-checking lock. Without them, in concurrent case, the second pattern is definitely better.

\$\endgroup\$
  • \$\begingroup\$ Stack.Pop might still destroy internal data structures when used concurrently without locks which makes Stack behave unpredictable. ConcurrentStack or locks. \$\endgroup\$ – Just another metaprogrammer Nov 20 '12 at 18:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.