Populating a ListView with Items

Question

I populate a ListView with Items. Each Item has data attached to its .Tag property.

Over time, I have needed to handle many cases where something would be null or missing or corrupt or whatever.

And so was born this beast:

private void PlayRecording()
{
    if (list.Items.Count >= 1)
    {
        if (list.SelectedItems.Count >= 1)
        {
            if (((Item)list.SelectedItems[0].Tag).Attachments.Count >= 1)
            {
                if (((Item)list.SelectedItems[0].Tag).Attachments[0].AudioStreams.Count >= 1)
                {
                    if (((Item)list.SelectedItems[0].Tag).Attachments[0].AudioStreams[0].AudioStream != null)
                    {
                        byte[] recording = ((Item)list.SelectedItems[0].Tag).Attachments[0].AudioStreams[0].AudioStream;

                        if (recording != null)
                        {
                            using (MemoryStream memoryStream = new MemoryStream(recording))
                            {
                                if (memoryStream != null)
                                {
                                    soundPlayer.Stream = memoryStream;
                                    soundPlayer.Play();

                                    AudioState = AudioStates.Playing;
                                }
                                else status.Text = "The Memory Stream returned is null. The stream may be corrupt or non-existent.";
                            }
                        }
                        else status.Text = "Notes is unable to play the recording.";
                    }
                    else status.Text = "The selected item's Audio Stream is null. The recording may be corrupt or missing.";
                }
                else status.Text = "The selected item does not contain any recordings to play.";
            }
            else status.Text = "The selected item does not contain any attachments.";
        }
        else status.Text = "You haven't selected an item in the list.";
    }
    else status.Text = "There are no items in the list.";
}

Yeah, sure, I could just get rid of the if's and else's and replace ((Item)list...) with Item item = list... and wrap it all in a single Try{}Catch{}, but I really do hate using Try{}Catch{} unless there is something I am not expecting. And to be honest, using Try{} Catch{} makes me feel lazy and like I'm not doing it properly.

Is there any way I can improve this code and make it a little smaller (while still handling all of the cases where I would need to convey a message)?

Answer 1

Yeah, sure, I could just get rid of the if's and else's and replace ((Item)list...) with Item item = list... and wrap it all in a single Try{}Catch{}, but I really do hate using Try{}Catch{} unless there is something I am not expecting. And to be honest, using Try{} Catch{} makes me feel lazy and like I'm not doing it properly.

That is true. The best exception is the exception which will be avoided.

That beeing said, I need to tell you that you are using the arrow antipattern here.

How about reversing the conditions by using guard clauses and return early like

if (list.Items.Count == 0)
{
    status.Text = "There are no items in the list.";
    return;
}
if (list.SelectedItems.Count == 0)
{
    status.Text = "You haven't selected an item in the list.";
    return;
}  

........  

or maybe add a method which returns a string for a not satisfied condition and an empty string for a satisfied condition like so you can place these guard clauses in a separate method.

---

If I read recording I think about a boolean. Maybe it is only me, but I would rename the variable to audioContent or recordedContent to be more clear.

---

Avoiding braces {} for single instruction ifand else can lead to serious problems. I would like to encourage you to always use them.

---

Btw, your method is doing too many things and is therefor violating the single responsibiliy principle. The method is validating conditions, updating the GUI and also playing the audio stream.

Answer 2

I might have missed it in one of the other answers, but in this code:

using (MemoryStream memoryStream = new MemoryStream(recording))
{
    if (memoryStream != null)

memoryStream will never be null, so the check is redundant.

Answer 3

You check if there are one or more selected items but the the rest of your code only works on the first selection. I think you should specifically check for a single selected item. I've used SingleOrDefault from the System.Linq namespace.

I wonder about the value you get from saying the list is empty - surely that's just the same behaviour as not having selected anything from the list?

It might also be better to invert all of your if conditions to reduce indentation:

private void PlayRecording()
{
    var selectedItem = list.SelectedItems.SingleOrDefault();
    if (selectedItem == null)
    {
        status.Text = "...";
        return;
    }
    var attachment = ((Item)selectedItem.Tag).Attachments.SingleOrDefault();
    if (attachment == null)
    {
        status.Text = "...";
        return;
    }
    var audioStream = attachment.AudioStreams.SingleOrDefault();
    if (audioStream == null) 
    {
        status.Text = "...";
        return;
    }
    var playableStream = audioStream.AudioStream;
    if (playableStream == null)
    {
        status.Text = "The selected item's Audio Stream is null. The recording may be corrupt or missing."; 
        return;
    }
    PlayStream(playableStream);
}

private void PlayStream(AudioStream audioStream)
{
    byte[] recording = audioStream;

    if (recording != null)
    {
        using (var memoryStream = new MemoryStream(recording))
        {
            if (memoryStream != null)
            {
                soundPlayer.Stream = memoryStream;
                soundPlayer.Play();
                AudioState = AudioStates.Playing;
                return;
            }
            status.Text = "The Memory Stream returned is null. The stream may be corrupt or non-existent.";
            return;
         }
     }
     status.Text = "Notes is unable to play the recording.";
}

Answer 4

You should place the properties in variables and call the variable in the inner if/else statements. Otherwise in every if statement the condition has to step through every property again and again (((Item)list.SelectedItems[0].Tag)...). Here's how you can do this:

private void PlayRecording()
{
    if (list.Items.Count >= 1)
    {
        if (list.SelectedItems.Count >= 1)
        {
            var tag = (Item)list.SelectedItems[0].Tag
            if (tag.Attachments.Count >= 1)
            {
                var attachment = tag.Attachments[0];
                if (attachment.AudioStreams.Count >= 1)
                {
                    var audioStream = attachment.AudioStreams[0].AudioStream;
                    if (audioStream != null)
                    {
                        //Executing the rest of your code
                    }
                    else status.Text = "The selected item's Audio Stream is null. The recording may be corrupt or missing.";
                }
                else status.Text = "The selected item does not contain any recordings to play.";
            }
            else status.Text = "The selected item does not contain any attachments.";
        }
        else status.Text = "You haven't selected an item in the list.";
    }
    else status.Text = "There are no items in the list.";
}

---

The var keyword:

Note that I used the var keyword. From the C# Programming Guide:

The var keyword can also be useful when the specific type of the variable is tedious to type on the keyboard, or is obvious, or does not add to the readability of the code.

So lines like:

MemoryStream memoryStream = new MemoryStream(recording);

would become:

var memoryStream = new MemoryStream(recording);

---

Following check is redundant:

if (((Item)list.SelectedItems[0].Tag).Attachments[0].AudioStreams[0].AudioStream != null)
{
    byte[] recording = ((Item)list.SelectedItems[0].Tag).Attachments[0].AudioStreams[0].AudioStream;

    if (recording != null)
}

You check if the AudioStream is not null, if it isn't you assign it to a variable and then check for null again on that variable.

---

Following way of working won't really shorten your code as it is not eally possible. But, by splitting validation and executing code if the input is valid, it makes your code more readable:

private void PlayRecording()
{
    var message = "";
    byte recording[];

    if(IsListViewValid(list, out message, out recording))
    {
        using (var memoryStream = new MemoryStream(recording))
        {
            if (memoryStream != null)
            {
                soundPlayer.Stream = memoryStream;
                soundPlayer.Play();
                AudioState = AudioStates.Playing;
            }
            else status.Text = "The Memory Stream returned is null. The stream may be corrupt or non-existent.";
        }
    }
    else status.Text = message;
}

private bool IsListViewValid(ListView list, out string message, out byte[] recording)
{
    if (list.Items.Count >= 1)
    {
        if (list.SelectedItems.Count >= 1)
        {
            var tag = (Item)list.SelectedItems[0].Tag
            if (tag.Attachments.Count >= 1)
            {
                var attachment = tag.Attachments[0];
                if (attachment.AudioStreams.Count >= 1)
                {
                    var audioStream = attachment.AudioStreams[0].AudioStream;
                    if (audioStream != null)
                    {
                        message = "";
                        recording = audioStream;
                        return true;
                    }
                    else message = "The selected item's Audio Stream is null. The recording may be corrupt or missing.";
                }
                else message = "The selected item does not contain any recordings to play.";
            }
            else message = "The selected item does not contain any attachments.";
        }
        else message = "You haven't selected an item in the list.";
    }
    else message = "There are no items in the list.";
    return false;
}

Answer 5

May be you could consider to make some constraints in the UI (the list view/form), so the user won't be able to select items with no streaming data. The five outmost if-statements are all concerned with missing data on an item selected by the user. It will give a better user experience, and you will be able to separate the input verification from the program execution.