6

It has already been determined here that an empty using block is not an appropriate way to override Dispose(), but what about the following case?

Is this a legitimate use for an empty using block?

try
{
    using (File.OpenRead(sourceFile)) { }
}
catch (FileNotFoundException)
{
    error = "File not found: " + sourceFile;
}
catch (UnauthorizedAccessException)
{
    error = "Not authorized to access file: " + sourceFile;
}
catch (Exception e)
{
    error = "Error while attempting to read file: " + sourceFile + ".\n\n" + e.Message;
}

if (error != null)
    return error;

System.Diagnostics.Process.Start(sourceFile);
8
  • You're just checking if you can access and read the file, you don't actually need the contents? Jul 25, 2014 at 14:32
  • Since using basically expands to a try { ... } finally { ... }, what you're doing will result in something like try { try { ...} finally { ... } } catch ( ... ) { ... } catch ( ... ) { ... } catch ( ... ) { ... }.
    – Corak
    Jul 25, 2014 at 14:37
  • @MichaelMcGriff I excluded it for simplicity. Edited to provide a bit more context, but it's not relevant to the discussion. Jul 25, 2014 at 14:37
  • @Dan context is almost always relevant :) Why don't you just try to start the process and catch any exceptions there? Jul 25, 2014 at 14:38
  • 1
    I'd be ok with it if you had a comment. Otherwise you can use this pattern Jul 25, 2014 at 14:43

2 Answers 2

3

No it's not a legitimate use for an empty using block.

You can simply write the try like this:

try
{
    File.OpenRead(sourceFile).Close();
}

Either OpenRead() will succeed and return a stream that you must close (or dispose, but I think .Close() better expresses your intent), or it will thrown an exception and not return anything.

Therefore you can always just close the returned value.

(I assume that you are just checking for read access before doing something else. However, be aware that you could in theory have a race condition because the accessibility of the file could change between you making this check and later on actually opening the file. This is unlikely to happen in practice, but you should be aware of the possibility.)

6
  • But if OpenRead throws it gets not closed. Jul 25, 2014 at 14:42
  • @TimSchmelter If OpenRead throws, it doesn't get opened so there's nothing to close. Jul 25, 2014 at 14:42
  • @TimSchmelter If OpenRead() throws, the file didn't get opened and it doesn't return anything. Jul 25, 2014 at 14:43
  • @MatthewWatson: ah ok, but personally i find this not readable. It's not obvious on the first sight that this cannot cause an open resource. That's why i also prefer a using (or try-catch-finally) here. I would also use using (MethodThatReturnsDisposable()){ } because the execution of the method could be important but i don't want to consume the returned disposable. Jul 25, 2014 at 14:43
  • 1
    @MatthewWatson: i haven't said that it's not obvious that it cannot be null, i've said that i must give it some thought to notice that this will close always and only if there is no exception and the object is not null. It's obvious now, yes. What i also like with the using is that it is not a method like Close where you need to think about null or not null. The intent is clear, call the method, do nothing with the returned object, ensure that it gets disposed always. Jul 25, 2014 at 14:55
-1

Not much sense in using empty using block, as you simply can add finally at the end of catch chains and handle Dispose there:

FileStream fs;
try {

    fs = File.OpenRead(sourceFile); 
}
catch(..) {
}
catch(..) {
}
catch(..) {
}
finally {

  if(fs !=null) {
      fs.Close();
      fs.Dispose();

  }
}
5
  • 1
    This still seems incredibly pointless as all he's seemingly trying to do is see if the file exists and he has access to it. Not a good use of OpenRead.
    – Dave Zych
    Jul 25, 2014 at 14:34
  • @DaveZych: seems it's not only for checking presence of file. but more, in this case you have also possible feedback about a cause of the problem.
    – Tigran
    Jul 25, 2014 at 14:39
  • @DaveZych It checks for any errors that would prevent the user from reading the file, not just File.Exists(). Jul 25, 2014 at 14:41
  • Note that this code won't actually compile unless you initialise fs to null. Jul 25, 2014 at 14:54
  • @MatthewWatson: yes, also you can not use catch(..). this is just a pseudocode, to provide an idea.
    – Tigran
    Jul 25, 2014 at 15:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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