views:

149

answers:

5

I'm working on an ASP.NET MVC app, designing the domain models, using (testing) the new EF Code First feature.

I have an Activity entity that may or may not have a Deadline, what is the best way to approach it?

1 property:

public DateTime?  Deadline {get; set;}
and check vs null before using

or

2 properties:

public DateTime Deadline {get; set;}
public bool HasDeadline  {get; set;}

At first I thought of the first option, but then I started thinking that maybe the second option would be better regarding the DB...

Is there any best practice regarding this?

+8  A: 

I'd go with the first option. After all, it's exactly an encapsulated form of the second.

The encapsulation makes it clear that you've only got one logical value (or lack thereof). In the second form you can treat the properties as if they were entirely independent, which they're logically not.

In terms of the database, I'd expect the first form to be just as easy too... presuambly you'll have a nullable DATETIME field in the database, won't you? It should map directly.

Jon Skeet
I'd go with the first option if `HasDeadline` affects only the `Deadline` column. If it affects multiple fields or the behavior, then a separate column might be appropriate (is there an `OverduePenalty` column that only makes sense if there is a deadline?). Think of whether you'd have a lot of queries along the lines of `WHERE NOT ISNULL('Deadline')`, because `WHERE 'HasDeadline'` feels a lot cleaner.
Ben Voigt
Since hes doing code-first on a domain model, who cares what the SQL looks like? Make the domain pretty, and usable, then worry about the database side of it.
Michael Shimmins
The difference comes across in LINQ as well.
Ben Voigt
Thanks, its good to see my first thought was good... darn second thoughts.. as an extra thing, what about implementing a HasDeadline property that has no setter and returns Deadline!=null? just to "pretty up" the code?
Francisco Noriega
lol, already suggested. Also lets you add checks in there such as `deadline must be after this date and not in the past` kind of thing
TerrorAustralis
Why bother? If you're going to use a nullable type anyway, why wrap the value of one of its properties in another one? Just another hoop to jump through, especially when debugging. You might as well make another property `public DateTime DeadlineValue { get { return Deadline.Value ?? DateTime.MinValue; } }`
Michael Shimmins
@Michael: Your first comment about "who cares what the SQL looks like?" really is not a good approach to take when coding. You should always be looking down the road at what future code, or implementations will look like. Why save time now just to waste it 2 fold later on cleaning up your own mess? That is nothing more than lazy in my opinion.
Adkins
@Adkins - I think thats a bit of an outdated perspective. Yes a view to the future is important, however with refactoring tools they way they are now, and the relative ease with which we can makes changes to well designed and constructed systems, the impact of making the wrong decision isn't as great as it perhaps may have been with legacy code. A loosely coupled, highly cohesive system will support changes fairly easily. In this specific instance though, I was assuming we're using an ORM of some sort of map this entity to a table. Since it will be taking care of the SQL for us - who cares.
Michael Shimmins
+1  A: 

I would use the first option. In the long run the second option will probably cause some maintenance problems because you have to remember to check and use both of the properties.

Also one option is to use one property but instead of making it nullable, you could return a Null object (also known as Special Case).

Mikael Koskinen
A: 

The database is used to storing NULL values - storing a Min value in the databsae, and then having a flag to indicate if you should trust that value makes queries complicated.

I like nullable types since the reflect the domain's intent - there is no date, not 'there isn't a date, so pretend the first of January 1970 means no date'.

There is also an overhead of maintaining the HasDealine value - you need to set it each time the corresponding property is updated. Also how do you clear it? If you set the Deadline to a date, it will set the HasDeadline to true. How do I 'unset' it? Would you set HasDeadline to false, but leave the Deadline field intact with the previous value?

Overall icky.

Michael Shimmins
All these problems exist with `Nullable<T>` as well, and the solution of "throw an exception from the Value getter if HasValue is false" isn't all that great, IMHO.
Ben Voigt
What do you mean they exist? Since I can assign `null` to a `Nullable<T>` type I have a really simple way to 'clear' the value. Since I can't assigned `null` to a `DateTime` how do I indicate that the entity no longer has a Deadline set? And of course, never throw an exception for something that isn't exceptional. Thats why the HasValue is there, so you can evaluate it before doing something. My point is that HasValue on Nullable<T> is all wired up for you. Its easy, good to go, and doesn't require repetitive plumbing each you want to use it.
Michael Shimmins
You can't actually assign `null` to a `Nullable<T>`, the compiler is lying to you. You're actually assigning `default(Nullable<T>)` (i.e. default constructed `Nullable<T>`) which, being that it is a value type, means raw binary zeroing -- `HasValue` becomes false, and `Value` becomes `default(T)` whatever the all-zeros value of `T` means.
Ben Voigt
My point exactly. From a writing code perspective, I can now assign null to a DateTime, and it takes care of setting the HasValue for me. If you do this yourself, rather than using a Nullable<T> type, you need to implement this logic each time. Why re-invent the wheel?
Michael Shimmins
+1  A: 

You should use the nullable, as it does exactly what you want. Using two separate properties means that you lose the connection between them, and you need to explain with documentation that they have a relation.

The nullable type should also fit better against a database type, however you should first design your object for how it works as an object, not for how you will store it in the database. If the use of a database generation tool causes you to make bad decisions when designing the code, it's contra-productive.

Guffa
+2  A: 

How about a combination of both just for the sake of making your code more readable?

public DateTime? Dealine{get; set;}
public bool HasDeadline
{
    get
    {
        return (Deadline != null);
    }
}

Its easy to read and does exactly the same thing that the consumer would have to do anyway. Besides...

if(HasDeadline)
    doStuff();

is easier to read than

if(Dealine != null)
    doStuff();

:)

TerrorAustralis
What about: `if(Deadline.Hasvalue)` as opposed to `if(HasDeadline)`. You don't need to compare to null since the `Nullable<T>` has a built in `HasValue` method.
Michael Shimmins