HiveBrain v1.2.0
Get Started
← Back to all entries
debugcsharpModerate

Populating a ListView with Items

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withitemspopulatinglistview

Problem

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 = "

Solution

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.

Code Snippets

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

........

Context

StackExchange Code Review Q#90616, answer score: 18

Revisions (0)

No revisions yet.