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 often have static classes which uses the DataContext somehow, and they all make a new DataContext, but often i allready have one instantiated elsewhere, so why not use that?. And then i would do this:

public static bool SignIn(string email, string password, DataContext db = null)
{

    bool disposeDb = false;
    if (db == null)
    {
        disposeDb = true;
        db = new DataContext();
    }

    //Sign In stuff, or any other stuff...

    if (disposeDb) db.Dispose();
}

But is there a cool trick to do this? or am i just coding it wroing? or is it ok?

Update

Ok so the DataContext is not expensive to instantiate. i guess i just leave it be.

But here is another example: (You might kill me for making other flaws here aswell, please do!)

public static User GetUser(DataContext db = null)
{
    if (HttpContext.Current.Request.Cookies["Id"] == null)
    {
        return null;
    }
    else
    {
        bool disposeDb = false;
        if (db == null)
        {
            disposeDb = true;
            db = new DataContext();
        }

        int id = Convert.ToInt32(HttpContext.Current.Request.Cookies["Id"].Value);
        string cookieKey = HttpContext.Current.Request.Cookies["CookieKey"].Value;

        User user = db.Users.FirstOrDefault(x => x.Id == id && x.CookieKey == cookieKey);

        if (disposeDb) db.Dispose();
        return user;
    }
}

This is again nice because if i allready have a context i can provide it, and then i won't have to attach the entity(User) to another context if i want to modify it. for instance:

 User user = MyClass.GetUser(db);
 user.Email = "asdsasd";
 db.SaveChanges();

How can i impoove? or is it okay?

Update 2 The DataContext should only be instansiated once for pr page request. And here is a neat way to do, that actually allows me to access it even from static methods:

http://stackoverflow.com/a/10153406/973485

share|improve this question
    
I believe that ... since db is not passed in be reference, it should be disposed automatically as it falls out of scope when the function exists. Why do you sometimes pass in an instantiated DataContext, and why do you want it to auto-dispose. Is it good or bad? Well, it depends. What are you trying to do? –  Leonid Dec 2 '12 at 0:01
    
Not sure if i follow, Well if there is a DataContext paramater is provided, it is because it is instansiated elsewhere, and will be disposed wlsewhere, but if you dont have one, and only need the result of that method, please dispose after use. –  BjarkeCK Dec 2 '12 at 0:11
2  
How expensive is it to create a new DataContext? If it is expensive, then perhaps you create t once and hold on to it. Does the library provide the connection pooling for you? I do not like your original code (nothing personal), but I cannot tell you exactly how to make it better because I do not see examples of how you are using it. It is weird to me that the DataContext may or may not be already created. What control that? –  Leonid Dec 2 '12 at 0:26
1  
"I Often have static classes" ??? I would look into why you often have these. Too many static classes IMHO can sometimes hints at design problems. Perhaps these are causing you to write code that you are not that keen about? –  dreza Dec 2 '12 at 0:32
    
@Leonid Not that expensise. so i'll just instasiate those inside. dreza Okay, i don't have that many static classes! –  BjarkeCK Dec 2 '12 at 0:54
show 1 more comment

4 Answers

up vote 3 down vote accepted

I see 2 issues with SignIn method:

  • it is static. It means that you can't unit-test business logic that uses SignIn functionality in isolation, you'll have to hit the database in your tests.
  • DataContext in web applications should usually be instantiated once per web request because in most cases web request represents a unit-of-work, and as such should be covered by a single transaction. If you're using ASP.NET MVC you can move all the logic that deals with DataConext instantiation and calling SaveChanges/Dispose into base controller class, and expose instance of DataContext as a property. That way you business logic will be cleaned from DataContext management, and most likely you won't actually need a separate class with methods like SignIn since the functionality can go right into Controller class.

About GetUser method - if this a method is in Controller class, and taking previous bullets into consideration, the code would look like that:

//MyBaseController exposes DataContext property
public class UserController : MyBaseController
{
    public User GetUser()
    {
        HttpCookie idCookie = Request.Cookies["Id"];
        HttpCookie keyCookie = Request.Cookies["CookieKey"];

        int id;
        if (idCookie == null || keyCookie == null || !int.TryParse(idCookie.Value, out id))
            return null;

        return DataContext.Users.FirstOrDefault(x => x.Id == id && x.CookieKey == keyCookie.Value);
    }
}
share|improve this answer
    
I'm going to accept this one as the answer becuase it let me to research how a you can make one DataContext pr Request, and i found this stackoverflow.com/a/10153406/973485 to be my answer. –  BjarkeCK Dec 2 '12 at 11:22
add comment

At the very least, you should place the second if statement in a finally block. As things stand, the code is not generally correct from a purely mechanical perspective. I would also phrase it as:

bool dbDispose = db == null;
db = db ?? new DataContext();

From a design point of view: yes, I'd say this violates the single responsibility principle. You have a method that not only does certain calculations, but also creates a DataContext if necessary. If creation has to be modified at any point, then you'll have many places to change that. (Unlikely if it's default-constructed like this, but I doubt it's that simple in practice.)

It looks to me like SignIn should actually be a member function of DataContext.

share|improve this answer
    
Wow, to make SignIn a part of DataContext would be just wrong... just.. or would it? I can't imagen thats how people make theese kind of functions.. –  BjarkeCK Dec 2 '12 at 0:53
    
Making it directly part of a DataContext could be wrong, if that gives DataContext too much responsibility. On the other hand, having a class with a member DataContext and a SignIn method could definitely make sense. –  Anton Golov Dec 2 '12 at 9:46
add comment

So, default argument values are nice, but in this case I favor the old-school (multiple methods with the same name) approach

public static bool SignIn(string email, string password)
{
    using (var db = new DataContext())
    {
        return SignIn(email, password, db);
    }
}

public static bool SignIn(string email, string password, DataContext db)
{
    Require.IsNotNull(db, "nice error message here"); // Or whatever the syntax is ...
    //Sign In stuff, or any other stuff...
}

... and so on ...

I like static methods, Rich Hickey likes static methods. Static methods are one honking good idea! Let's have more of these. The static methods hatas will keep on hating.

share|improve this answer
1  
whole MakeDataContextIfNeeded method can be rewritten as db ?? new DataContext(), there is no need is this method. –  almaz Dec 2 '12 at 9:49
    
Do you have any sources that the cleanup you expect is likely to happen? –  Anton Golov Dec 2 '12 at 10:36
    
@Anton Golov, I talked out of my ass. It is generally a bad idea to let an IDisposable fall out of scope, so much so that there is a rule specifically for it. msdn.microsoft.com/en-us/library/ms182289(v=vs.100).aspx Apparently when compiling C++ to .Net, you can rely on the fact that a finalizer will run which will also call Dispose. When using C#, this will not happen for some time. So, an open file will be locked until the GC decides to run. It smells like an undefined behavior and you can code a Schroedingers cat logic around the fact that you do not know whether the file is locked –  Leonid Dec 3 '12 at 5:26
add comment

Instead of init DataContext inside static methods you can use Singleton pattern or Factory pattern depending on what kind of life cycle of the object DataContext. That patterns allows to encapsulate logic of DataContext initialization and provide more control over the life cycle of an instance(including implementation caching politics).

As for me correct variant GetUser method should look similar on that:

public static class UserService{

public static User GetUser(HttpContext context, DataContext db)
{
   if (db==null)
      throw new ApplicationException("Data Context is null");
   if (context==null)
      throw new ApplicationException("Http Context  is null");

    if (context.Request.Cookies["Id"] == null)
    {
       return null;
    }
    else
    {
         int id = Convert.ToInt32(context.Request.Cookies["Id"].Value);
         string cookieKey = context.Request.Cookies["CookieKey"].Value;

         User user = db.Users.FirstOrDefault(x => x.Id == id && x.CookieKey == cookieKey);

         return user;
    }

}

}
share|improve this answer
add comment

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.