current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite

I've created a create a console app that would:

  1. Call a method to check an email account (I've done this step)
  2. Convert the attachment to pdf (I've done this step)
  3. Then once the conversion is complete wait 30 seconds
  4. Repeat the previous 3 steps continuously

I've done Step 1 and 2 int the ProcessMailMessages() method. The following code works but I want to know if I am on the right track or is there a better way to poll a email client?

    private static int secondsToWait = 30 * 1000;

    static void Main(string[] args)
    {
        bool run = true;
        do
        {
            try
            {
                Task theTask = ProcessEmailTaskAsync();
                theTask.Wait();
            }
            catch (Exception e)
            {
                Debug.WriteLine("<p>Error in Client</p> <p>Exception</p> <p>" + e.Message + "</p><p>" + e.StackTrace + "</p> ");
            }
            GC.Collect();

        } while (run);

    }

    static async Task ProcessEmailTaskAsync()
    {
        var result = await EmailTaskAsync();
    }

    static async Task<int> EmailTaskAsync()
    {
        await ProcessMailMessages();
        await Task.Delay(secondsToWait);
        return 1;
    }

    static async Task ProcessMailMessages()
    {
        ...............................................................................
    }
share|improve this question
2  
Why are you calling CG.Collect();? – Trevor Pilley Nov 15 '12 at 16:50
up vote 1 down vote accepted

I would rather do smth. like that (I added CancellationToken in case you would want to be able to cancel email parsing while it's in progress):

private static TimeSpan timeToWait = TimeSpan.FromSeconds(30);

//somewhere in your code:
_cancellationTokenSource = new CancellationTokenSource();

private static void Main(string[] args)
{
    CancellationToken token = _cancellationTokenSource.Token;
    do
    {
        try
        {
            ProcessEmailsAndWaitAsync(token).Wait();
        }
        catch (Exception e)
        {
            Debug.WriteLine("<p>Error in Client</p> <p>Exception</p> <p>" + e.Message + "</p><p>" + e.StackTrace + "</p> ");
        }
    } while (!token.IsCancellationRequested);
}

private static async Task ProcessEmailsAndWaitAsync(CancellationToken token)
{
    await ProcessMailMessages(token);
    await Task.Delay(timeToWait, token);
}

private static async Task ProcessMailMessages(CancellationToken token)
{
    //...............................................................................
}
share|improve this answer
up vote 0 down vote
  1. Calling GC.Collect() shouldn't be necessary except in very specific circumstances. Don't do it if you don't have a very good reason.
  2. Why are do you have unused local variable in ProcessEmailTaskAsync()? It's not useful for anything. What's more, what is the whole point of the method? You can Wait() on Task<int> too.
  3. It's not clear what does the int returned from EmailTaskAsync() mean. If it's supposed to represent status, an enum would be much better. If you do need to use int for some reason, you really should document what each value means. And in your case, you always return the same value and it doesn't seem to be used anywhere, so you should just change the return type to Task and don't return anything.

  4. Using async-await won't give you any benefit in your case. Basically, there are two reasons for using async-await:

    1. It can use smaller number of threads, improving efficiency.
    2. It can avoid blocking the UI thread, improving responsiveness.

    Neither of those reasons is relevant in your case (you don't have a UI thread and using async-await this way will use more threads, not less, because of the Wait()), so you should consider whether using async-await will give you any benefit at all.

share|improve this answer
    
Hi Svick, Thanks for the constructive feedback, Are you able to provide an alternative implementation? – user1825645 Nov 19 '12 at 5:50
    
Why? Are any of my suggestions unclear? – svick Nov 19 '12 at 19:21
up vote -2 down vote

I believe you could use a much more elegant solution using the ContineWith method on the Task object

Here is some basic pseudo code...

function RecursiveEmailPollRoutine()
{
    var PollEmailTask = new Task(() => {
                                       //Poll the email
                                       //Pause for your delay
                                   }).ContinueWith(()=> {
                                                            //check your cancel var
                                                            //and cancel if need be
                                                            //otherwise recursively 
                                                            //call this function
                                                        }).Start();
)

This hasn't been tested but you get the gist. I hope this helps.

share|improve this answer
4  
Please don't link to your blog in every (or any) answer. Linking to a specific blog post that's relevant to the question is fine as long as you summarize the content of the post in your answer, but just linking to your blog without any indication of how it's relevant to the question is against the rules. – sepp2k Nov 15 '12 at 18:12
1  
Are you seriously saying this code is more readable than using async-await? The whole point of that is to avoid code like this. Besides, your code won't work, because you can't Start() the Task returned from ContinueWith(). On the other hand, you have to Start() the Task if you create it directly using an constructor. – svick Nov 17 '12 at 10:54

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.