Instantiating if null

Question

I often have static classes which uses the DataContext somehow, and they all make a new DataContext, but often I already have one instantiated elsewhere, so why not use that?

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 wrong? Or is it ok?

Update

The DataContext is not expensive to instantiate. I guess I just leave it be.

But here is another example:

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 already 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:

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

How can I improve it? Or is it okay?

Update 2

The DataContext should only be instantiated once per page request. And here is a neat way to do it, that actually allows me to access it even from static methods.

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

almaz · Accepted Answer · 2012-12-02 08:43:19Z

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

Anton Golov · Answer 2 · 2012-12-02 00:19:21Z

up vote 5 down vote

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.

answered Dec 2 '12 at 0:19

Anton Golov

2,8891127

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 a comment |

Leonid · Answer 3 · 2012-12-03 05:27:34Z

up vote 0 down vote

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.

edited Dec 3 '12 at 5:27

answered Dec 2 '12 at 4:18

Leonid

77959

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 a comment |

Frank59 · Answer 4 · 2012-12-02 11:20:39Z

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;
    }

}

}

asked	4 years ago
viewed	920 times
active	2 years ago

current community

your communities

more stack exchange communities

Instantiating if null

4 Answers 4

Your Answer

Not the answer you're looking for? Browse other questions tagged c# entity-framework asp.net-mvc-4 null or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

Instantiating if null

4 Answers 4

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c# entity-framework asp.net-mvc-4 null or ask your own question.

Related

Hot Network Questions