debugcsharpModerate
Populating a ListView with Items
Viewed 0 times
withitemspopulatinglistview
Problem
I populate a ListView with Items. Each Item has data attached to its
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 = "
.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
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
Avoiding braces
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.
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.