I am not a very experienced programmer. So I ask.
The field in question is the “Amount” field. I store an amount only in a couple levels of the derived class.
The code is a slice of the POCO classes that are making up my data model, the datalayer is Entity framework, code first.
I mention that because the reason I did the amount property like I did is because when I placed the Amount just in a derived class or two and not in the Navigation class, the table in the database would have an extra field for each class with an Amount property, like (Amount, Amount1, Amount2). I did not like that.
So after screwing around with the code for awhile I discovered that…
Placing the Amount property in the navigation class and overriding it in the classes that implemented Amount gave me a table with just one “Amount” field that was Null, unless it was one of the classes that used amount in which case it had a value, which was just what I wanted. I liked that.
So the question is, is the way the Amount property is handled completely proper or have some downside?
public class Navigation
{
int Id { get; set; }
string Title { get; set; }
string Description { get; set; }
ObservableCollection<Navigation> Children { get; set; }
public virtual decimal? Amount
{
get { return null; }
set { value = null; }
}
}
public class C1 : Navigation { }
public class C2 : Navigation { }
public class C3 : Navigation
{
private decimal _amount;
public override decimal? Amount
{
get { return _amount; }
set { if (value != null) _amount = (decimal)value; }
}
}
public class C4 : Navigation
{
private decimal _amount;
public override decimal? Amount
{
get { return _amount; }
set { if (value != null) _amount = (decimal)value; }
}
}
14
It is a not good practice because people expect something when they see the property in the class. Imagine if someone writes:
var n = new Navigation (...);
n.Amount = 123.45;
......
Console.WriteLine(n.Amount); //null? why???
A better approach would be to at least warn them:
public virtual decimal Amount{
get { return new NotSupportedException("Amount is not supported in this class."); }
set { //same as above }
}
But now you’d get the C# practice warning: “properties should behave like fields and should never throw exceptions”.
Perhaps you can add a Type field, then throw away inheritance altogether. But I’m not sure if this is appropriate for your scenario (maybe you have lots of extra fields in derived classes).
Of course, if you’re not writing a library that will be used by lots of other programmers, there’s nothing wrong to bend common practices. Be flexible.
Do you find it confusing to yourself? If it works and you’re comfortable with it, go ahead.
What would I do? This is one of the down sides of Entity Framework, which I don’t really like anyway. To me, it brings more problems than it solves. I’d just create tables in the DB, create the classes as you wish in C#, then write some helper methods to map them. If all you do is SELECT and INSERT, it’s not as hard as you may think. It’s outside the scope of this site, but I guarantee you it’s around ~50 lines (I’ve done it).
2
One thing you can do if you want to add a property to only a few of the subclasses is to use another interface. In your example: Navigation -> AmountEnabledNavigation -> C3, but Navigation -> C1. In the code that handles the objects, e.g. adds the amounts, you can simply check if the object inherits from AmountEnabledNavigation and then decide based on that.
This also prevents later issues like exceptions when accessing the objects. Although these can be caught and handled, they slow down code considerably.