current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite
1

I used the information from the article "Auto-Start ASP.NET Applications (VS 2010 and .NET 4.0 Series)" to create a background task that runs on an infinite loop. Ultimately, this program is going to be consuming a REST API to get data from a remote web site and insert records into my database. For the time being, I am just doing a basic insert.

public class PreWarmCache : System.Web.Hosting.IProcessHostPreloadClient
{
    private static BackgroundTaskContext db = new BackgroundTaskContext();
    private static Timer _timer;

    public void Preload(string[] parameters)
    {
        // Perform initialization and cache loading logic here...
        try
        {
            var doStuff = new DoStuff();
            doStuff.DoStuffName = "Test from PRELOAD INIT " + DateTime.Now;
            db.DoStuffs.Add(doStuff);
            db.SaveChanges();
        }
        catch
        {
        }

        var timeInMilliseconds = 0;
        _timer = new Timer(Callback, null, timeInMilliseconds, Timeout.Infinite);

    }

    public static void Callback(Object state)
    {
        var timeInMilliseconds = 5000;
        Stopwatch watch = new Stopwatch();

        watch.Start();

        // Update the database
        try
        {
            var doStuff = new DoStuff();
            doStuff.DoStuffName = "Test from CALLBACK " + DateTime.Now;
            db.DoStuffs.Add(doStuff);
            db.SaveChanges();
        }
        catch
        {
        }

        _timer.Change(Math.Max(0, timeInMilliseconds - watch.ElapsedMilliseconds), Timeout.Infinite);
    }
}
share|improve this question
    
Interesting. I've been keeping an eye on the Auto-Start feature but haen't gotten a chance to use it much. Any idea how it does with these limitations of IIS applications? –  George Mauer Jun 24 '14 at 18:24
    
Not yet. I am just trying to get the REST API code working. Debugging is difficult. I am not sure how to debug this once deployed to IIS and the code does not seem to be working. I know I can attach a worker process. But, it's not really helping me. –  Allan Horwitz Jun 24 '14 at 20:29
    
heavy logging always helps me in that scenario. –  George Mauer Jun 24 '14 at 20:56
    
Is there a question to answer here? –  Pierre Menard Jul 18 '14 at 20:27
2  
It's a code review. The question is, "Could this code be better?" –  Allan Horwitz Jul 18 '14 at 23:33

2 Answers 2

up vote 2 down vote

Naming

I don't like underscore notation, but it's regularly debated and is mostly a matter for personal preference, however everybody agrees that if you pick a naming scheme, stick with it.

private static BackgroundTaskContext db = new BackgroundTaskContext();
private static Timer _timer;

It should be _db or timer. Not both.

Static

Your use of static is confusing here. Do you really only want one timer and background task context for every instance of PreWarmCache? If you only want one instance, make it so, do not allow multiples and then have them all overwriting each other.

There also seems to be a case where calling Preload multiple times will just reset the timer and only call callback with the state from the last preload call.

Var

Mostly, you're pretty good for using var correctly, but you don't use it consistently.

Stopwatch watch = new Stopwatch();

Should be

var watch = new Stopwatch();
share|improve this answer
up vote 2 down vote

There are some things that you could have done better certainly.

My first pick is the class name DoStuff. Please try pick a more meaningful name like Action or Task.

In this class you seem to be appending the current time to the property DoStuffName (which should also be renamed accordingly). You could pass the name in the constructor and append the current time instead, like:

public class Task{
   public Task(string name){
      Name = name + " " + DateTime.Now;
   }
   public string Name{
     get; private set;
   }  
}

You also seem to have one of the most evil bad practises of programming - the empty catch block. Be sure to, at least, log the exception somewhere.

share|improve this answer
    
You should preferably throw the exception again, even if you log it. Unless you've handled the situation that caused the exception, you should rethrow the exception. –  Nick Udell Dec 2 '14 at 12:02

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.