Empty "using" block

Question

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);

You're just checking if you can access and read the file, you don't actually need the contents? — Michael McGriff, Jul 25 '14 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 '14 at 14:37
@MichaelMcGriff I excluded it for simplicity. Edited to provide a bit more context, but it's not relevant to the discussion. — Dan Bechard, Jul 25 '14 at 14:37
@Dan context is almost always relevant :) Why don't you just try to start the process and catch any exceptions there? — Michael McGriff, Jul 25 '14 at 14:38
@MichaelMcGriff Process.Start doesn't throw the same exceptions as OpenRead, so it's harder to determine what went wrong. — Dan Bechard, Jul 25 '14 at 14:39
I'd be ok with it if you had a comment. Otherwise you [can use this pattern](http://stackoverflow.com/a/937558/119477) — Conrad Frix, Jul 25 '14 at 14:43
@ConradFrix There are many other exceptions besides IOException that might be thrown. — Dan Bechard, Jul 25 '14 at 14:45
@Dan. Yep so you'd have to add them. The point was that using the finally block might remove some of the smell of the `using` inside a `try` — Conrad Frix, Jul 25 '14 at 14:47

score 3 · Answer 1 · answered Jul 25 '14 at 14:39

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.)

answered Jul 25 '14 at 14:39

Matthew Watson

104,400
10
158
276

But if `OpenRead` throws it gets not closed. – Tim Schmelter Jul 25 '14 at 14:42
@TimSchmelter If `OpenRead` throws, it doesn't get opened so there's nothing to close. – Dan Bechard Jul 25 '14 at 14:42
@TimSchmelter If `OpenRead()` throws, the file didn't get opened and it doesn't return anything. – Matthew Watson Jul 25 '14 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. – Tim Schmelter Jul 25 '14 at 14:43
@TimSchmelter It really should be obvious though - you should know that `File.OpenRead()` only ever returns a non-null value. And if you know that, you should also know that you can always safely close it. If you don't know that, then do you check its return value for `null` before using it? Because if you are saying that it's not obvious that `OpenRead()` cannot return null then you would presumably be writing code that checks the return value for null... Like Tigran did in his code sample above. – Matthew Watson Jul 25 '14 at 14:50
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. – Tim Schmelter Jul 25 '14 at 14:55

Tigran · Answer 2 · 2014-07-25T14:40:09.187

-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();

  }
}

edited Jul 25 '14 at 14:40

answered Jul 25 '14 at 14:33

Tigran

61,654
8
86
123

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 '14 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 '14 at 14:39
@DaveZych It checks for any errors that would prevent the user from reading the file, not _just_ `File.Exists()`. – Dan Bechard Jul 25 '14 at 14:41
Note that this code won't actually compile unless you initialise `fs` to null. – Matthew Watson Jul 25 '14 at 14:54
@MatthewWatson: yes, also you can not use catch(..). this is just a pseudocode, to provide an idea. – Tigran Jul 25 '14 at 15:01

Empty "using" block

2 Answers2