debugcsharpMinor
Should this be written with exception handling instead of nested if-thens?
Viewed 0 times
thishandlingwrittenexceptionwithinsteadnestedshouldthens
Problem
I have a function that iterates through a list of file updates, and for each file, if updates are needed, download the file from the server, make the updates, and upload the updated version. Each update gets marked with a string identifying the result of the update operation.
There are several steps in the update process where I would not want updating to continue for that file. Right now I have a lot of nested if-then statements, so that the iteration won't proceed any further for that file and will just mark the issue and go on to the next file. Code as follows:
```
public void updateAllFiles()
{
foreach (string fileName in updateList.fileNameList())
{
updatesForFile = updateList.updatesForFile(fileName);
hyperlinkEntry = hyperlinkList.getHyperlinkEntry(fileName);
if (hyperlinkEntry == null)
{
updatesForFile.markResultAllRows("File not found on server");
}
else
{
hyperlink = hyperlinkEntry.hyperlink;
if (updatesForFile.updateNeeded())
{
if (!po.checkout(hyperlink))
{
updatesForFile.markResultAllRows("checked out to other user");
}
else
{
downloadPath = downloadFolder + fileName;
downloadResult = dl.download(hyperlink, downloadPath);
if (downloadResult == false)
{
updatesForFile.markResultAllRows("download failed");
}
else
{
file = new FileObject(xlApp, downloadPath, updatesForFile);
if (file == null)
{
updatesForFile.markResultAllRows("unable to open");
There are several steps in the update process where I would not want updating to continue for that file. Right now I have a lot of nested if-then statements, so that the iteration won't proceed any further for that file and will just mark the issue and go on to the next file. Code as follows:
```
public void updateAllFiles()
{
foreach (string fileName in updateList.fileNameList())
{
updatesForFile = updateList.updatesForFile(fileName);
hyperlinkEntry = hyperlinkList.getHyperlinkEntry(fileName);
if (hyperlinkEntry == null)
{
updatesForFile.markResultAllRows("File not found on server");
}
else
{
hyperlink = hyperlinkEntry.hyperlink;
if (updatesForFile.updateNeeded())
{
if (!po.checkout(hyperlink))
{
updatesForFile.markResultAllRows("checked out to other user");
}
else
{
downloadPath = downloadFolder + fileName;
downloadResult = dl.download(hyperlink, downloadPath);
if (downloadResult == false)
{
updatesForFile.markResultAllRows("download failed");
}
else
{
file = new FileObject(xlApp, downloadPath, updatesForFile);
if (file == null)
{
updatesForFile.markResultAllRows("unable to open");
Solution
Maybe simple loop logic can serve your purposes better. If what you want is to ignore the rest of the loop body if a condition is met, the
If what you need is to exit altogether when a condition is met, then I'd favor
Exceptions are very useful, but they are very expensive, so unless you don't really care about the performance of this code, I'd favor a solution based in flow control any day of the week.
continue statement will follow immediately with the next iteration.foreach (string fileName in updateList.fileNameList())
{
updatesForFile = updateList.updatesForFile(fileName);
hyperlinkEntry = hyperlinkList.getHyperlinkEntry(fileName);
if (hyperlinkEntry == null)
{
updatesForFile.markResultAllRows("File not found on server");
continue;
}
hyperlink = hyperlinkEntry.hyperlink;
if (updatesForFile.updateNeeded())
{
if (!po.checkout(hyperlink))
{
updatesForFile.markResultAllRows("checked out to other user");
continue;
}
downloadPath = downloadFolder + fileName;
downloadResult = dl.download(hyperlink, downloadPath);
if (downloadResult == false)
{
updatesForFile.markResultAllRows("download failed");
continue;
}
file = new FileObject(xlApp, downloadPath, updatesForFile);
if (file == null)
{
updatesForFile.markResultAllRows("unable to open");
continue;
}
foreach (FeatureUpdate featureUpdate in updatesForFile.getFeatureUpdateList())
{
resultValue = featureUpdate.getColumnValue("result");
if (!resultValue.Contains("updated"))
{
file.update(featureUpdate, updateMethods.lowFeatureLength);
updatesForFile.markResult(featureUpdate.getIndex(), "updated");
}
}
file.close();
updateList.save();
}
}If what you need is to exit altogether when a condition is met, then I'd favor
return, maintaining the same logic as the continue example. This would also give a lot more flexibility to refactor your code into functions and even probably use of kind of error codes, if it gets to that point.Exceptions are very useful, but they are very expensive, so unless you don't really care about the performance of this code, I'd favor a solution based in flow control any day of the week.
Code Snippets
foreach (string fileName in updateList.fileNameList())
{
updatesForFile = updateList.updatesForFile(fileName);
hyperlinkEntry = hyperlinkList.getHyperlinkEntry(fileName);
if (hyperlinkEntry == null)
{
updatesForFile.markResultAllRows("File not found on server");
continue;
}
hyperlink = hyperlinkEntry.hyperlink;
if (updatesForFile.updateNeeded())
{
if (!po.checkout(hyperlink))
{
updatesForFile.markResultAllRows("checked out to other user");
continue;
}
downloadPath = downloadFolder + fileName;
downloadResult = dl.download(hyperlink, downloadPath);
if (downloadResult == false)
{
updatesForFile.markResultAllRows("download failed");
continue;
}
file = new FileObject(xlApp, downloadPath, updatesForFile);
if (file == null)
{
updatesForFile.markResultAllRows("unable to open");
continue;
}
foreach (FeatureUpdate featureUpdate in updatesForFile.getFeatureUpdateList())
{
resultValue = featureUpdate.getColumnValue("result");
if (!resultValue.Contains("updated"))
{
file.update(featureUpdate, updateMethods.lowFeatureLength);
updatesForFile.markResult(featureUpdate.getIndex(), "updated");
}
}
file.close();
updateList.save();
}
}Context
StackExchange Code Review Q#43960, answer score: 4
Revisions (0)
No revisions yet.