Creating a new camera stream

Question

I'm currently in the process of refactoring some code and I was wondering if I should use switch or if-else for the is code:

Switch

 for (int i = 0; i < length; i++)
 {
 CameraStream stream = null;
 var id = (Guid)CameraGridView.Rows[i].Cells[0].Value;
 var Camerasettings = Settings.GetCameraSettings(id);
 switch (i)
 {
 case 0:
 stream = new CameraStream(Camerasettings, new List<PictureBox> { Camera1a, Camera1b }, 
 new List<Label> { Camera1aLbl, Camera1bLbl });
 break;
 case 1:
 stream = new CameraStream(Camerasettings, new List<PictureBox> { Camera2a, Camera2b },
 new List<Label> { Camera2aLbl, Camera2bLbl });
 break;
 case 2:
 stream = new CameraStream(Camerasettings, new List<PictureBox> { Camera3a, Camera3b },
 new List<Label> { Camera3aLbl, Camera3bLbl });
 break;
 case 3:
 stream = new CameraStream(Camerasettings, new List<PictureBox> { Camera4a, Camera4b },
 new List<Label> { Camera4aLbl, Camera4bLbl });
 break;
 case 4:
 stream = new CameraStream(Camerasettings, Camera5, Camera5Lbl);
 break;
 case 5:
 stream = new CameraStream(Camerasettings, Camera6, Camera6Lbl);
 break;
 case 6:
 stream = new CameraStream(Camerasettings, Camera7, Camera7Lbl);
 break;
 case 7:
 stream = new CameraStream(Camerasettings, Camera8, Camera8Lbl);
 break;
 case 8:
 stream = new CameraStream(Camerasettings, Camera9, Camera9Lbl);
 break;
 }
 cameraStreams.Add(stream);
 stream.Start();
 }

if

 for (int i = 0; i < length; i++)
 {
 CameraStream stream = null;
 var id = (Guid)CameraGridView.Rows[i].Cells[0].Value;
 var Camerasettings = Settings.GetCameraSettings(id);
 if (i == 0)
 {
 var PicBoxes = new List<PictureBox> { Camera1a, Camera1b };
 var Labels = new List<Label> { Camera1aLbl, Camera1bLbl };
 stream = new CameraStream(Camerasettings, PicBoxes, Labels);
 }
 if (i == 1)
 {
 var PicBoxes = new List<PictureBox> { Camera2a, Camera2b };
 var Labels = new List<Label> { Camera2aLbl, Camera2bLbl };
 stream = new CameraStream(Camerasettings, PicBoxes, Labels);
 }
 if (i == 2)
 {
 var PicBoxes = new List<PictureBox> { Camera3a, Camera3b };
 var Labels = new List<Label> { Camera3aLbl, Camera3bLbl };
 stream = new CameraStream(Camerasettings, PicBoxes, Labels);
 }
 if (i == 3)
 {
 var PicBoxes = new List<PictureBox> { Camera4a, Camera4b };
 var Labels = new List<Label> { Camera4aLbl, Camera4bLbl };
 stream = new CameraStream(Camerasettings, PicBoxes, Labels);
 }
 if (i == 4)
 {
 stream = new CameraStream(Camerasettings, Camera5, Camera5Lbl);
 }
 if (i == 5)
 {
 stream = new CameraStream(Camerasettings, Camera6, Camera6Lbl);
 }
 if (i == 6)
 {
 stream = new CameraStream(Camerasettings, Camera7, Camera7Lbl);
 }
 if (i == 7)
 {
 stream = new CameraStream(Camerasettings, Camera8, Camera8Lbl);
 }
 if (i == 8)
 {
 stream = new CameraStream(Camerasettings, Camera9, Camera9Lbl);
 }
 cameraStreams.Add(stream);
 stream.Start();
 }

• Joel Harkes: remove the forloop and use if statements looks better to me.
• Konrad Morawski: I'm afraid neither version looks clean or maintainable. Proper refactoring in this case should go deeper than choosing between a wall of ifs and a wall of switch-cases.
• m.lanser: @BCdotWEB I'm using WinForms
• Pieter Witvoet: I'd put those picture boxes and labels into arrays (or arrays of arrays), so you don't need any checks and duplicated code inside your for loop. Alternately, write a utility method to simplify the creation, adding and starting of a singleCameraStream and just call it 8 times in a row: no loop or checks needed.
• Jeff.Clark: I agree with @KonradMorawski. Take a look at all the answers of (stackoverflow.com/questions/126409/…) and loot it's knowledge.

Answer 1

Most of your names are meaningless: Camera1a, Camera1aLbl, etc. barely inform us what they are.

---

Is there a reason why CameraStream has two different constructors? Why not simply have a single one which takes Camerasettings, List<PictureBox> and List<Label>, that way you can concentrate on filling List<PictureBox> and List<Label> and just have a single new CameraStream() at the end.

---

But I fear your refactoring might need to widen its scope, because even if you apply this you still have an ugly if or switch.

Answer 2

what about this?

 CameraStream stream = null;
 var id = (Guid)CameraGridView.Rows[i].Cells[0].Value;
 var Camerasettings = Settings.GetCameraSettings(id);
 if (length <= 0)
 {
 var PicBoxes = new List<PictureBox> { Camera1a, Camera1b };
 var Labels = new List<Label> { Camera1aLbl, Camera1bLbl };
 var stream = new CameraStream(Camerasettings, PicBoxes, Labels);
 cameraStreams.Add(stream);
 stream.Start();
 }
 if (length <= 1)
 {
 var PicBoxes = new List<PictureBox> { Camera2a, Camera2b };
 var Labels = new List<Label> { Camera2aLbl, Camera2bLbl };
 stream = new CameraStream(Camerasettings, PicBoxes, Labels);
 cameraStreams.Add(stream);
 stream.Start();
 } ...

This way you remove the loop but keep the functionality, am i correct?

you can encapsulate the double code in a function with input of camera and camera label.