I'm finishing an unfinished C# WinForms project, which is a save game editor for an unspecified game. I noticed that character portraits and icons' IDs have a valid range of 1 to 905, but there was no code to verify that the character ID is within that range, so I opted to fix it. I have an in-progress solution to this, with the code for it (with a lot of function names partially redacted) at the end of this post, and wanted to know if it would work. Visual Studio says there are no formatting errors, but I know that alone will not quite be enough. I cannot compile to test because I am not quite at a stage where the code is compilable, with a lot of missing WinForms data. If the specific exception that I placed would not be appropriate, what exception would best fit? If a while loop should not be used in this way, what method should I use that doesn't require rewriting the entire code block from scratch? Here is the relevant code I have so far:
void getImage(int slot)
{
short ID = 0;
if (slot == 1)
{
try
{
ID = convertStringtoID(comboBoxSlot1ID.Text);
}
catch (System.FormatException)
{
ID = 0;
}
while (ID > 0 && ID < 906)
{
if (ID < 10)
{
pictureBoxSlot1Portrait.Image = (Image)Properties.Resources.ResourceManager.GetObject("00" + ID);
pictureBoxSlot1Icon.Image = (Image)Properties.Resources.ResourceManager.GetObject("icon00" + ID);
}
else if (ID > 9 && ID < 100)
{
pictureBoxSlot1Portrait.Image = (Image)Properties.Resources.ResourceManager.GetObject("0" + ID);
pictureBoxSlot1Icon.Image = (Image)Properties.Resources.ResourceManager.GetObject("icon0" + ID);
}
else if (ID > 99 && ID < 906)
{
pictureBoxSlot1Portrait.Image = (Image)Properties.Resources.ResourceManager.GetObject("" + ID);
pictureBoxSlot1Icon.Image = (Image)Properties.Resources.ResourceManager.GetObject("icon" + ID);
}
else
{
try { }
catch (System.IndexOutOfRangeException)
{
ID = 0;
}
}
}
}
}
I have not yet been able to try or test anything yet; this is merely a question regarding the valid use of C# code. I also saw just 1 question that was similar, but not the same, as my question in the 'possibly related questions' section before posting this, although it did partially indicate that what I am doing may be okay. I am expecting the code to throw IndexOutOfBoundsException if ID is greater than 905, and read the proper image file if not.
Edit: Okay, I have revised the code significantly, using feedback posted here. I have ended up with the following code, which shows no errors in Visual Studio, and despite others saying 'it is improper to use exceptions to control flow', works for my use case. Also, I am technically not using them to control anything, just that I want an exception window to pop up for anyone who has a save file with a character ID greater than 905, so that they know that their save file is likely corrupted. I want the program to still be able to continue operation afterward, and not have this error be fatal. Here is the revised code:
void getImage(int slot)
{
short ID = 0;
if (slot == 1)
{
try
{
ID = convertStringtoID(comboBoxSlot1ID.Text);
}
catch (System.FormatException)
{
ID = 0;
}
if (ID < 10)
{
pictureBoxSlot1Portrait.Image = (Image)Properties.Resources.ResourceManager.GetObject("00" + ID);
pictureBoxSlot1Icon.Image = (Image)Properties.Resources.ResourceManager.GetObject("icon00" + ID);
}
else if (ID > 9 && ID < 100)
{
pictureBoxSlot1Portrait.Image = (Image)Properties.Resources.ResourceManager.GetObject("0" + ID);
pictureBoxSlot1Icon.Image = (Image)Properties.Resources.ResourceManager.GetObject("icon0" + ID);
}
else if (ID > 99 && ID < 906)
{
pictureBoxSlot1Portrait.Image = (Image)Properties.Resources.ResourceManager.GetObject("" + ID);
pictureBoxSlot1Icon.Image = (Image)Properties.Resources.ResourceManager.GetObject("icon" + ID);
}
else if (ID > 905)
{
throw(System.InvalidOperationException);
ID = 0;
continue;
}
}
}
ID = convertStringtoID(comboBoxSlot1ID.Text);
This is a custom method. Rather than wrap it in a try/catch, make it return the default 0 value. While I'm here, that comboBox should also be given as an argument to the getImage() method (and the potriat/icon pair returned as a result, maybe in a tuple).
I understand you might not have that option, and assuming you have a legitimate need to use it instead of a simple int.Parse() or int.TryParse(), or if it's used in other places and expected to throw. In that case, I then look at the catch value and see this:
ID = 0
... and then notice this:
while (ID > 0 && ID < 906)
Put them together, and we can just exit the method right away.
try
{
ID = convertStringtoID(comboBoxSlot1ID.Text);
}
catch ()
{
return;
}
Looking more at this loop:
while (ID > 0 && ID < 906)
Nothing inside that loop ever changes ID, so the loop would run forever. You want if() instead of while().
I also see the conditional expressions used to decide how many 0s you need to prefix the ID value. You can replace all that like this:
pictureBoxSlot1Portrait.Image = (Image)Properties.Resources.ResourceManager.GetObject($"{ID:000}");
pictureBoxSlot1Icon.Image = (Image)Properties.Resources.ResourceManager.GetObject($"dot{ID:000}");
Another item, for the question about the empty try:
else
{
try { }
catch (System.IndexOutOfRangeException)
{
ID = 0;
}
}
The try has nothing in it, least of all anything that would ever throw an exception, and therefore the catch will NEVER run. It seems like you just want this:
else
{
ID = 0;
}
And I need to caution here about using exceptions for program logic control flow in the first place. It's extremely poor practice. Rather, exceptions should reserved for events that are genuinely exceptional or unexpected.
But really, I'd skip that else() completely, since we already know logically from the checks made when entering the block that the value is in the expected range.
Finally, I saw this comment:
slot has a valid range of 1 to 3 ... after the section of code I listed is the exact same code posted twice more, only used for slots 2 and 3 instead.
We can improve on that like this:
if (--slot < 0 || slot > 2) return;
var portraits = new PictureBox[] {pictureBoxSlot1Portrait, pictureBoxSlot2Portrait, pictureBoxSlot3Portrait};
var icons = new PicutureBox[] {pictureBoxSlot1Icon, pictureBoxSlot2Icon, pictureBoxSlot3Icon};
var combos = new ComboBox[] {comboBoxSlot1ID, comboBoxSlot2ID, comboBoxSlot3ID};
And then use the new array variables when needed:
ID = convertStringtoID(combos[slot].Text);
// ...
portraits[slot].Image = (Image)Properties.Resources.ResourceManager.GetObject($"{ID:000}");
icons[slot].Image = (Image)Properties.Resources.ResourceManager.GetObject($"dot{ID:000}");
Even better if the arrays are defined for this at the class level, to avoid fresh allocations each time this runs, but I'll leave that change for another time.
Put it all together, and the method reduces to this:
void getImage(int slot)
{
if (--slot < 0 || slot > 2) return; //also adjusts from 1-based slot to 0-based array index
var portraits = new PictureBox[] {pictureBoxSlot1Portrait, pictureBoxSlot2Portrait, pictureBoxSlot3Portrait};
var icons = new PicutureBox[] {pictureBoxSlot1Icon, pictureBoxSlot2Icon, pictureBoxSlot3Icon};
var combos = new ComboBox[] {comboBoxSlot1ID, comboBoxSlot2ID, comboBoxSlot3ID};
short ID = 0;
try {
ID = convertStringtoID(combos[slot].Text);
} catch() { return; }
if (ID <= 0 || ID >= 906) return;
portraits[slot].Image = (Image)Properties.Resources.ResourceManager.GetObject($"{ID:000}");
icons[slot].Image = (Image)Properties.Resources.ResourceManager.GetObject($"dot{ID:000}");
}
This now works with only about 1/6 the code of the original and saves three levels of indentation. That it will also be faster is just a bonus.
There are further improvements that can be made as well, to better decouple the UI from the back end and simplify the code, but this is enough for today. But it's worth repeating here that the only exception ever thrown or even tested is for the convertStringtoID() method, which could likely be rewritten to avoid the need. No other try/catch is needed.