Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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");
                            }
                            else
                            {
                                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();
                            }
                        }
                    }
                }
            }
        }

    }

Because it's getting harder to read the code, I'm wondering if I should use exception handling here instead, and if so, how it should be implemented here--e.g., should an exception be thrown in getHyperlinkEntry() and caught in updateAllFiles(), or should it be thrown and caught in updateAllFiles() like this?

try
{
    hyperlinkEntry=hyperlinkList.getHyperlinkEntry(fileName);
    if (hyperlinkEntry == null)
    {
        throw new UpdateException("File not found on server");
    }

    ///more function calls and exception throwing here
}
catch (UpdateException ue)
{
    updatesForFile.markResultAllRows(ue.updateMessage);
}

Or are the existing nested if-then clauses more appropriate for this situation?

share|improve this question

3 Answers 3

up vote 3 down vote accepted

You can use continue instead:

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");
            continue;
        }

        hyperlink = hyperlinkEntry.hyperlink;
        if (updatesForFile.updateNeeded())
        {
            if (!po.checkout(hyperlink))
            {
                updatesForFile.markResultAllRows("checked out to other user");
                continue;
            }
            downloadPath = downloadFolder + fileName;
            ... etc ...

Alternatively you can move the code into a subroutine and use return when you want to exit early.

My if-then implementation allowed me to perform some cleanup tasks within the for-loop after all the if-thens were evaluated (similar to the finally of a try-catch structure); is that possible using this continue approach?

The only 'cleanup task' which I see in the original code is the file.close statement:

                        file = new FileObject(xlApp, downloadPath, updatesForFile);
                        if (file == null)
                        {
                            updatesForFile.markResultAllRows("unable to open");
                        }
                        else
                        {
                            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();
                        }

That part can be rewritten to use 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 you have cleanup tasks that might be a good time to invent a subroutine:

 file = new FileObject(xlApp, downloadPath, updatesForFile);
 updateFeatures(file); // <-- new subroutine call
 file.close();
 updateList.save();

The new subroutine can use return instead of continue, whenever it wants to stop processing:

void updateList(File file)
{
    if (!someCondition)
        return; // <-- early return
    somethingElse();
    somethingFurther();
}

When the subroutine returns, whether early or late, then your cleanup tasks run.

share|improve this answer
    
My if-then implementation allowed me to perform some cleanup tasks within the for-loop after all the if-thens were evaluated (similar to the finally of a try-catch structure); is that possible using this continue approach? –  sigil Mar 10 at 19:20
    
@sigil Yeah, you can still use finally for that, though I'm not completely sure it's a good idea. –  svick Mar 10 at 19:20
    
@sigil I updated my answer to try to address your comment. –  ChrisW Mar 10 at 20:31
    
Thanks. In the meantime I implemented it using svick's finally recommendation; I'll try your subroutine approach if that doesn't work out. –  sigil Mar 10 at 20:35

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 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.

share|improve this answer
    
Good to know about the performance cost of exception throwing. –  sigil Mar 10 at 18:29
1  
@sigil Everyone says that exceptions are "very expensive"; but they are not entirely correct: see stackoverflow.com/a/891230/49942 –  ChrisW Mar 10 at 18:37
    
@ChrisW Nice, I guess nothing is black or white... –  ArthurChamz Mar 10 at 18:44
    
I'd word that performance note the other way around: Exceptions are somewhat expensive, but you should use them where appropriate anyway and use something else only if you really care about the performance of this code. (See also the famous quote from Knuth on premature optimization.) –  svick Mar 10 at 19:24

You should really not use Exceptions for flow control. They are inefficient, and can be their own readability headache. Exceptions should be used for exceptional events, such as an IO failure, or your app not finding a file it really should have. The use of continues, as ChrisW notes, is a great way to avoid this, and arranging them like guard clauses (or guard-esque clauses) could aid readability:

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");
            continue;
        }

        if (!updatesForFile.updateNeeded())
        {
            continue;
        }

        hyperlink = hyperlinkEntry.hyperlink;
        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();
    }
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.