CODEWITHSUNDEEP
c#exceptiontry-catch
hl3mukkel
hl3mukkel

Reputation: 6049

Catch exception appropriate when disposing of the result?

Consider the following piece of code:

public static Stream OpenWavStream(string path)
{
    var stream = new FileStream(path, FileMode.Open);

    try
    {
        stream.Seek(44, SeekOrigin.Current);
    }
    catch (Exception)
    {
        stream.Dispose();
        throw;
    }

    return stream;
}

I'm opening a wav stream whose data always starts at offset 44. If seeking to that offset fails, the stream is disposed, otherwise it is returned. Considering that catch (Exception) is considered bad practice, is it appropriate in this case?

Should one rather research the concrete exceptions (even though the stream should be disposed if any kind of exception happens within the Stream.Seek call) or move it into a finally block?

Upvotes: 0

Views: 56

Answers (3)

user6293065
user6293065

Reputation:

Only if the Stream fails to load. Something I use:

string fileName = "C:\\PCM.wav";

if (!System.IO.File.Exists(fileName))
{
LogStatus("Wave file not found.");
return;
}
else
{
WaveFileByteArray = File.ReadAllBytes(fileName);
LogStatus("Wave file Loaded!" + Environment.NewLine);
}

This works fine.

then to play/Use:

System.Media.SoundPlayer soundPlayer;
soundPlayer.Stream.Seek(0, SeekOrigin.Begin);
soundPlayer.Stream.Write(WaveFileByteArray, 0, WaveFileByteArray.Length);
soundPlayer.Play();

Relying on an Exception to catch possible errors, like is presented, is not best practice, unless the error is unexpected. Dealing with possible Errors before they occur is best practice.

Upvotes: 2

Rob
Rob

Reputation: 27367

catch (Exception) is bad practice if you are using it to swallow exceptions and not handling them. You are immediately re-throwing the exception, and doing it properly (you're not doing throw ex; for example). You will need to dispose the stream for any exception, so you should not catch specific exceptions here.

Your code is perfectly fine. However, I am skeptical about the usefulness of the method. Without seeing the rest of the application, it might make sense for the callee to create the stream within a using block, even with a helper method.

//In your callee code
using (var stream = new FileStream(path, FileMode.Open))
{
    ConfigureStream(steam);
    //Other stuff..
}

public static void ConfigureStream(Stream stream)
{
    stream.Seek(44, SeekOrigin.Current);
}

Or, you could check the length of the stream first, to avoid the exception entirely.

Upvotes: 1

Hakunamatata
Hakunamatata

Reputation: 1275

Why not use a using block in calling method and leave the closing of steam to the system. using will close the stream even if there is an exception. 

public static Stream OpenWavStream(string path)
    {
        var stream = new FileStream(path, FileMode.Open);
        stream.Seek(44, SeekOrigin.Current);
        return stream;
    }

public static void UseWaveStream()
    {
        try
        {
            using(Stream thisStream = OpenWavStream("C:\\myfile.txt"))
            {
                 // do whatever 
            }
        }
        catch(Exception ex)
        {
            Console.WriteLine(ex.ToString());
        }
    }

Upvotes: 1

Related Questions