current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I have a WPF MVVM Application. I have a Customer Model which is generated by an entity framework (Database first approach) and one CustomerViewModel where I created an instance of a Customer Model and used Model members inside the ViewModel property get and set.

Here is my Customer Model:

public partial class TblCustomer
{
    public TblCustomer()
    {
        this.TblSalesInvoices = new HashSet<TblSalesInvoice>();
        this.TblSalesOrders = new HashSet<TblSalesOrder>();
    }

    public int CustomerId { get; set; }  //Primary key Identity
    public string CustomerName { get; set; }
    public string Address1 { get; set; }
    public string Address2 { get; set; }
    public string City { get; set; }
    public string State { get; set; }
    public string Country { get; set; }
    public string Phone { get; set; }
    public string Email { get; set; }

    public virtual TblUser TblUser { get; set; }
    public virtual ICollection<TblSalesInvoice> TblSalesInvoices { get; set; }
    public virtual ICollection<TblSalesOrder> TblSalesOrders { get; set; }
}

Here is my CustomerViewModel:

    private TblCustomer_customerModel;
    public TblCustomer CustomerModel
    {
        get
        {
            return _customerModel?? (_customerModel= new TblCustomer());
        }
        set { _customerModel= value; }
    }

    #region Bindable Properties

    public string CustomerName
    {
        get
        {
            return CustomerModel.CustomerName;
        }
        set
        {
            CustomerModel.CustomerName= value;
            RaisePropertyChanged("CustomerName");
        }
    }

    public string Email
    {
        get
        {
            return CustomerModel.Email;
        }
        set
        {
            CustomerModel.Email = value;
            RaisePropertyChanged("Email");
        }
    }

    private ICommand _saveCommand;
    public ICommand SaveCommand
    {
        get
        {
            if (_saveCommand == null)
            {
                _saveCommand = new RelayCommand(() => Save());
            }
            return _saveCommand;
        }
    }
   #Endregion Bindable Properties


    private void SaveCustomer()
    {
        using(var context=new R_MaizeEntities())
        {
            context.TblCustomers.Add(CustomerModel);
            context.SaveChanges();
        }
    }

Is this a correct way create a Model class instance in ViewModel class and set and get model class members inside the ViewModel Property get and set? By doing so, does the ViewModel class have a dependency on the Model class? Is there any other efficient way to handle this situation?

share|improve this question

1 Answer 1

up vote 3 down vote accepted

Entity

public partial class TblCustomer

The Tbl prefix belongs in the database, if it belongs anywhere at all. You have plenty of ways with Entity Framework, to map an entity type to a specific table, there's no need to carry database design mistakes into the code. Also entity types don't need to be partial.

One way could be to use annotations:

using System.ComponentModel.DataAnnotations.Schema;

[Table("TblCustomer")]
public class Customer
{
}

Another way could be to use the fluent API in the OnModelCreating method override of your DbContext:

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Entity<Customer>().ToTable("TblCustomer");
}

The most verbose way would be to create an EntityTypeConfiguration class:

public class CustomerMap : EntityTypeConfiguration<Customer>
{
    public CustomerMap()
    {
        ToTable("TblCustomer");
    }
}

And then to load that configuration into the model builder:

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Configurations.Add(new CustomerMap());
}

You'll probably want to prefer convention over configuration, so simple data annotations should do the trick.

This comment:

public int CustomerId { get; set; }  //Primary key Identity

Doesn't need to exist. Entity Framework understands [EntityName]Id as your primary key (also just Id works), by convention - the mapping is automagic.

If you prefer it to be explicit (/by configuration) you can, again, use data annotations:

[Key()]
public int CustomerId { get; set; }

..or configure it in OnModelCreating:

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Entity<Customer>().ToTable("TblCustomer");
    modelBuilder.Entity<Customer>().HasKey(e => e.CustomerId);
}

..or configure it in an EntityTypeConfiguration class:

public class CustomerMap : EntityTypeConfiguration<Customer>
{
    public CustomerMap()
    {
        HasKey(e => e.CustomerId);
        ToTable("TblCustomer");
    }
}

And then to load that configuration into the model builder:

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Configurations.Add(new CustomerMap());
}

The comment doesn't add anything new, and at worst it could be just a lie (because if you're using configurations the entity type itself doesn't know which property is a key). I'd just remove it.

ViewModel

#region Bindable Properties

Don't. Any property that is public is bindable. This comment/region can only turn into an eventual lie. Remove it.

using(var context=new R_MaizeEntities())

R_MaizeEntities is an awful name for a type. Syntax highlighting doesn't even pick it up as a C# type name, that's a sign. I don't know what the R_ stands for, and I'm pretty sure it's meaningless - the type should be named MaizeEntities or MaizeContext.

One problem you have in your ViewModel, is that you can't test it without hitting the database and actually performing a save: your ViewModel is strongly coupled with everything you're newing up in that class.

Think of it: anything the ViewModel is newing up, is a dependency. If it makes sense for the ViewModel to be tightly coupled with specific commands, being tightly coupled with a specific data context makes your code pretty much untestable.

I would either constructor-inject the context (as an abstraction), or constructor-inject a context factory (as an abstract factory) whose job would be to instantiate a data context - and unit tests can inject a stub factory that instantiates a mock context for testing purposes.

The CustomerName property should be using _customerModel - it being null wouldn't result in a NullReferenceException at runtime, only as a silent binding error that you wouldn't be getting if _customerModel was constructor-injected instead of being new'd up in the property getter.

I think there might be a bug here:

_saveCommand = new RelayCommand(() => Save());

Looks like the command should be calling the SaveCustomer() method. Or there's a Save() method somewhere else?

RaisePropertyChanged doesn't follow naming conventions: if the method does what it says it does, it should be named OnPropertyChanged.

Raising a PropertyChanged event is surely something that's common to all your ViewModel classes. Why not write a ViewModelBase abstract class, and shove the property-changing logic in there? See this post for some ideas, more specifically about referring to property names in a strongly-typed manner.

share|improve this answer
    
very useful,I have a doubt ,if I use CustomerModel as constructor injected,I fill all the CustomerModel properties and insert that into my database,then what should I do to insert another record ,Is to clear all the customerModel properties or just set it to null then reInstantiate it what is the best approach ? –  Angel Jun 2 '14 at 7:26

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.