1

I am a beginner programmer and feel like I am repeating code(DownloadFiles()) as in the below example:

var files = DownloadFiles();
var retryCount = 0;

while (files == null && retryCount < 3)
{
    retryCount++; 
    Console.WriteLine("Retrying {0} time", retryCount);
    files = DownloadFiles();       
}

My application is basically downloading files via a HttpWebrequest` and is supposed to retry the download 3 times if no files are retrieved.

I need your expert opinion here:

I am repeating code with DownloadFiles()

Could this snippet of code be written more efficiently?

This may seem trivial but I want to develop good programming practices.

1
  • Generally this sort of thing is probably fine. In this case you might want to look into a do...while loop May 29, 2014 at 15:50

5 Answers 5

1

You can use do ... while as Ben Aaronson already suggested:

TypeOfFiles files;
var retryCount = 0;

do
{
    files = DownloadFiles();       

    if (retryCount > 0)
    {
        Console.WriteLine("Retrying {0} time", retryCount);
    }
}
while (files == null && retryCount++ < 3)

I moved the retryCount++ to inside the while so your number doesn't get off (compared to do this inside the loop).

2
  • 1
    You cant use var without providing some context to the compile. OP's code doesn't print "Retrying" on first attempt. And, your code will only download 3 times whilst OP's code will do 4 times.
    – Xiaoy312
    May 29, 2014 at 15:55
  • @Xiaoy312: I don't think it runs 3 times, since the ++ on retryCount is done AFTER evaluating the expression (as opposed to ++retryCount). May 29, 2014 at 15:58
1

Another way to do it is:

List<File> files;  // replace with whatever type files is
var retryCount = 0;

while ((files = DownloadFiles()) == null && retryCount < 3)
{
    retryCount++; 
    Console.WriteLine("Retrying {0} time", retryCount);
}
2
  • @PatrickHofman The code in the OP already works. The whole point of the question is to create a more readable/maintainable solution. This solution is dramatically worse than the OP's in that regard. That the code works doesn't mean it's a good answer.
    – Servy
    May 29, 2014 at 16:56
  • @PatrickHofman So a code review that takes code and makes it worse is not a useful answer, it is in fact a not useful answer. There is a specific button one can use to indicate that an answer is not useful. It's the downvote.
    – Servy
    May 29, 2014 at 17:01
0

You are not repeating your code, you already encapsulated the Download logic in a method, and you are looping over this method.

Bad things would have been to inline the DownloadFiles method or to unroll the while loop.

0
??? files; //type returned by the function
var retryCount = 0;

do 
{
    files = DownloadFiles();

    if (retryCount > 0)
        Console.WriteLine("Retrying {0} time", retryCount);       
} while (files == null && retryCount++ < 3)
1
  • Please add some explanation to your code solution for future reference.
    – easwee
    May 29, 2014 at 16:23
0

a for loop may look neater:

var files = DownloadFiles();

for (int retryCount = 0; files == null && retryCount < 3; retryCount++)
{
    Console.WriteLine("Retrying {0} time", retryCount);
    files = DownloadFiles();       
}

edit: additionally: I normally hate "magic numbers" - constants without context, such as the 3 in the comparison, as it makes code less maintainable. This is particularly true if you want to give the user a choice of how many times to retry. I would normally use something like:

const maxRetries = 3;

and

retryCount < maxRetries

in the loop but thats up for debate.

0

Your Answer

Reminder: Answers generated by Artificial Intelligence tools are not allowed on Stack Overflow. Learn more

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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