2
\$\begingroup\$

Goal

I'd like to understand if this current method, of loading data and inserting data into a database I currently use is to be avoided?

Code

I drafted a simple application that loads data and inserts data, simple as that.

Model

public class Person
{
    public int PersonId { get; set; }
    public string Firstname { get; set; }
    public string Lastname { get; set; }
}

Database service

public interface IDatabaseService
{
    List<Person> GetPeople();
    void InsertPerson(Person person);
}

public class DatabaseService : IDatabaseService
{
    public List<Person> GetPeople()
    {
        using (MySqlConnection conn = new MySqlConnection("connection"))
        {
            string query = "SELECT * FROM People;";
            var details = conn.Query<Person>(query);

            return details.ToList();
        }
    }

    public void InsertPerson(Person person)
    {
        using (MySqlConnection conn = new MySqlConnection("connection"))
        {
            string query = "INSERT INTO People (Firstname, Lastname) VALUES (@Firstname, @Lastname);";
            conn.Execute(query, new { @Firstname = person.Firstname, @Lastname = person.Lastname });
        }
    }
}

ViewModel

public class MainViewModel
{
    // Property to be loaded with records
    public ObservableCollection<Person> People { get; set; }


    // Properties to create new Person
    public string Firstname { get; set; }
    public string Lastname { get; set; }

    // Contructor
    public MainViewModel()
    {
        // Create database service and assign it to the readonly property
        DatabaseService databaseService = new DatabaseService();
        _databaseService = databaseService;

        LoadData();

        InsertDataCommand = new RelayCommand(InsertData);
    }

    // Service
    private readonly DatabaseService _databaseService;

    // Load data method
    protected void LoadData()
    {
        if(People == null)
        {
            People = new ObservableCollection<Person>();
        }
        People.Clear();
        _databaseService.GetPeople().ForEach(record => People.Add(record));
    }

    // Command to add new Person
    public ICommand InsertDataCommand { get; }
    protected void InsertData(object param)
    {
        Person person = new Person()
        {
            Firstname = Firstname,
            Lastname = Lastname
        };
        _databaseService.InsertPerson(person);

        // Refresh
        LoadData();
    }
}

Question

In my view model, the way I clear the ObservableCollection and reload the data.. is this ok?

\$\endgroup\$
9
  • \$\begingroup\$ Entity Framework, Code First. \$\endgroup\$
    – aepot
    Commented Jul 9, 2021 at 13:17
  • \$\begingroup\$ Meanwhile opening/closing the connection to each query may be not efficient unless they a pooled (connection string params setup). \$\endgroup\$
    – aepot
    Commented Jul 9, 2021 at 13:19
  • \$\begingroup\$ Why Entity Framework just out of curiosity? I think using queries with dapper is relevantly easy in my opinion? \$\endgroup\$ Commented Jul 9, 2021 at 13:46
  • \$\begingroup\$ Looks like initially there was no sense to develop EF then. \$\endgroup\$
    – aepot
    Commented Jul 9, 2021 at 13:57
  • \$\begingroup\$ I recommend having a backend between client and data storage \$\endgroup\$
    – Anders
    Commented Jul 9, 2021 at 14:25

3 Answers 3

2
\$\begingroup\$

Add a Repository layer which would be between Data and Service layers. Never use Service directly to Data layer. The Service layer suppose to hold the business logic that would be applied on the Repository layer. Each Service can work with one or more repositories, but each repository should only work with one entity. Here is an example :

public class PersonRepository
{
    public List<Person> GetPeople() { /* Dapper code here */  }

    public void InsertPerson(Person person) { /* Dapper code here */  }
}

public class PersonService 
{
    private readonly PersonRepository _personRepo = new PersonRepository();
    
    public List<Person> GetPeople() 
    { 
        // if there is some business logic needed 
        // you can add it here 
        return _personRepo.GetPeople();
    }
    
    public List<Person> InsertPerson(Person person)
    { 
        // if there is some business logic needed 
        // you can add it here 
        _personRepo.InsertPerson(person);
    }   
}

Each Repository should validate the object against the entity requirements (e.g. database table).

Each Service should validate the object against the business logic requirements.

In your view model, class properties should be at the top of the class not in between, and sort them by private properties first, then public properties comes after.

This part :

 // Contructor
public MainViewModel()
{
    // Create database service and assign it to the readonly property
    DatabaseService databaseService = new DatabaseService();
    _databaseService = databaseService;

    LoadData();

    InsertDataCommand = new RelayCommand(InsertData);
}
   
// Load data method
protected void LoadData()
{
    if(People == null)
    {
        People = new ObservableCollection<Person>();
    }
    People.Clear();
    _databaseService.GetPeople().ForEach(record => People.Add(record));
}

can be simplified by this :

 // Contructor
public MainViewModel()
{
    _databaseService = new DatabaseService(); 
    LoadData();
    InsertDataCommand = new RelayCommand(InsertData); 
}

// Load data method
protected void LoadData()
{
    People = new ObservableCollection<Person>(_databaseService.GetPeople());
}
\$\endgroup\$
2
  • \$\begingroup\$ What is this pattern called? Using the Repository.. \$\endgroup\$ Commented Aug 19, 2021 at 9:07
  • \$\begingroup\$ @Squirrel.98 it has no official design pattern name that I'm aware of, but it's known by Repository/Service pattern. \$\endgroup\$
    – iSR5
    Commented Aug 19, 2021 at 11:34
2
\$\begingroup\$

The way you implemented it looks absolute fine to me.

I would initialized the ObservabaleCollection with object creation and make it as get-only property:

public ObservableCollection<Person> People { get; } = new ObservableCollection<Person>();

That avoids the risk of null reference exceptions or different collection objects.

Clearing and recreating all objects is the simple case. It works but has the down side that the selected person within the grid disapears when realoding the persons (however, that may be ok in your use case).

An more advanced way is to update exiting, remove deleted and insert new persons. That requires a lot more code (e.g. you have to implement a PersonVioewModel with property changed support) but has better user experience on the other hand.

\$\endgroup\$
2
  • \$\begingroup\$ I like your last comment - do you know any resources with an example of what you have described? I'd like to look into it. \$\endgroup\$ Commented Jul 12, 2021 at 14:49
  • 1
    \$\begingroup\$ You have just to synchronize the two lists instead of recreating them: If item is in target but not in source, remove it; If item is in target and source, update it; if item is in source but not in target, add it. Here is an explaination of PropertyChanged implementation if you are not familiar with it: c-sharpcorner.com/article/… \$\endgroup\$
    – JanDotNet
    Commented Jul 13, 2021 at 4:38
1
\$\begingroup\$

Some quick remarks:

  • PersonId (in Person) is IMHO not a great pattern. IMHO you shouldn't prefix a property name with its class name.

  • IDatabaseService is far to generic a name. I'd use PersonService and use that class to handle all Person-related queries. Then you can have more generic names like GetAll() instead of GetPeople().

  • Use descriptive names. details as in var details = conn.Query<Person>(query); is not the correct name.

  • I don't find InsertPerson a great method name. To me it should be something like Create (along with Edit and Delete or Remove).

  • Why do you do this:

      DatabaseService databaseService = new DatabaseService();
      _databaseService = databaseService;
    

    Why not simply do _databaseService = new DatabaseService();?

    (Also you should use Dependency Injection to inject such a service.)

  • Don't put pointless comments all over the place. Comments should be rare and should only be used to explain why something is implemented the way it is. If you need the comment // Load data method it means your method name is not clear enough.

\$\endgroup\$
2
  • \$\begingroup\$ I understand your points - but overall the .Clear() then .ForEach(...Add(record)) is that ok to use? \$\endgroup\$ Commented Jul 9, 2021 at 20:41
  • \$\begingroup\$ @Squirrel.98 It's been too long since I've done WPF, but to me iSR5's solution -- new ObservableCollection<Person>(_databaseService.GetPeople()); -- seems better: one line, no ForEach. \$\endgroup\$
    – BCdotWEB
    Commented Jul 12, 2021 at 13:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.