HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Instantiating if null

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
nullinstantiatingstackoverflow

Problem

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.

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#19217, answer score: 5

Revisions (0)

No revisions yet.