Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
    
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. –  avip Nov 15 '12 at 22:12
add comment

2 Answers 2

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.

share|improve this answer
3  
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. –  svick Nov 15 '12 at 21:23
add comment

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.

share|improve this answer
    
Stack.Pop might still destroy internal data structures when used concurrently without locks which makes Stack behave unpredictable. ConcurrentStack or locks. –  FuleSnabel Nov 20 '12 at 18:59
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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