4
\$\begingroup\$

I need to learn more about Entity Framework, so I created a database to store notes. The database definition is as follows:

public class User
{
    [Key]
    public int UserId { get; set; }

    [MaxLength(20)]
    public string UserName { get; set; }

    public virtual List<Note> Notes { get; set; }
}

public class Note
{
    [Key]
    public int NoteId { get; set; }

    [MaxLength(30)]
    public string Title { get; set; }

    [MaxLength(200)]
    public string Content { get; set; }

    public int UserId { get; set; }
    public virtual User User { get; set; }
}

public class NotesContext : DbContext
{
    public DbSet<User> Users { get; set; }
    public DbSet<Note> Notes { get; set; }
}

I wrote the following helper functions (I'm working off a Console Application for now):

class Program
{
    private static void AddUser(string userName)
    {
        using (var db = new NotesContext())
        {
            db.Users.Add(new User { UserName = userName });
            db.SaveChanges();
        }
    }

    private static void RemoveUser(int userId)
    {
        using (var db = new NotesContext())
        {
            var user = db.Users.FirstOrDefault(u => u.UserId == userId);
            if (user != null)
            {
                db.Users.Remove(user);
                db.SaveChanges();
            }
        }
    }

    private static void ClearUsers()
    {
        using (var db = new NotesContext())
        {
            db.Users.RemoveRange(db.Users);
            db.SaveChanges();
        }
    }

    private static void AddNote(int userId, string title, string content)
    {
        using (var db = new NotesContext())
        {
            db.Notes.Add(new Note { UserId = userId, Title = title, Content = content });
            db.SaveChanges();
        }
    }

    private static void RemoveNote(int noteId)
    {
        using (var db = new NotesContext())
        {
            var note = db.Notes.FirstOrDefault(u => u.NoteId == noteId);
            if (note != null)
            {
                db.Notes.Remove(note);
                db.SaveChanges();
            }
        }
    }

    private static void ClearNotes()
    {
        using (var db = new NotesContext())
        {
            db.Notes.RemoveRange(db.Notes);
            db.SaveChanges();
        }
    }
}

Other than the fact that it would be somewhat expensive to add or remove a set of users using these extensions, rather than getting one DB instance, updating it, and running SaveChanges() once, is there anything I should be doing differently?

\$\endgroup\$
  • \$\begingroup\$ Should User.UserId and Note.NoteId really be a publically settable property? \$\endgroup\$ – Rick Davin May 10 '17 at 19:31
  • \$\begingroup\$ For referential integrity, if you remove a user from NotesContext.Users shouldn't you also remove the reference to those same users from NotesContext.Notes? \$\endgroup\$ – Rick Davin May 10 '17 at 19:34
  • 3
    \$\begingroup\$ @RickDavin not only should, they must be publically settable. \$\endgroup\$ – t3chb0t May 10 '17 at 19:35
  • 1
    \$\begingroup\$ The database should be taking care of referential integrity. \$\endgroup\$ – cHao May 10 '17 at 19:53
  • 2
    \$\begingroup\$ @RickDavin If you try to remove a user who still has notes, it throws an exception. I'm perfectly fine with that. \$\endgroup\$ – Hosch250 May 10 '17 at 20:00
3
\$\begingroup\$

Foreign Key

Your foreign key is typed to List. I recommend using a HashSet to prevent adding the same entities twice to you navigation properties. I also recommend instantiating it, too, so that you don't get null reference exceptions on new Users that haven't been loaded from EF.

public virtual ICollection<Note> Notes { get; set; } = new HashSet<Note>()

Additionally, you can take advantage of EF Code First Conventions to not require the foreign key id fields or primary key id fields at all, and have EF imply them for you. This will lead to your entities most closely resembling your domain model, instead of DTOs.

public class User
{
    [MaxLength(20)]
    public string UserName { get; set; }

    public virtual ICollection<Note> Notes { get; set; } = new HashSet<Note>();
}

public class Note
{
    [MaxLength(30)]
    public string Title { get; set; }

    [MaxLength(200)]
    public string Content { get; set; }

    public virtual User User { get; set; }
}

public class NotesContext : DbContext
{
    public DbSet<User> Users { get; set; }
    public DbSet<Note> Notes { get; set; }
}

Structure

There isn't a ton wrong with your code being structured as it is. Anything extra would really be redundancy at this stage, but then it isn't very large at this stage.

The following are recommendations that don't make sense in a trivial project like this, but once you start involving substantial logic, they will be a huge benefit.

Put your logic in a class that makes sense

Program is doing too much, move your DB logic into a Notes or Users class. For command line apps, I prefer to use Program to bridge the chasm between the screaming maw of stringly-typed CMD world and the beautiful, shining crystal palaces of your OOP domain.

I like to use a Service Object for this.

That DbContext Issue

The approach of using a whole db context per operation would be an efficiency issue but you appear to be aware of that.

However, performance aside, I'd recommend against this for a number of reasons.

These methods creating their own db contexts is a violation of the Single Responsibility Principle. It shouldn't matter what DB store you've got in there or when it's saved. Leave that to the consumer of those methods. On top of that, you cannot swap out what DbContext is used in the case of testing.

Secondly the static methods are permanently affecting state. It is very difficult to test these methods without first creating an entire database.

To fix this, you should inject this DbContext dependency.

Lastly, there's a lot of duplication in the boilerplate of creating a DbContext, so we can extract that out to a single method. With it in a single method, it frees you to be more open to altering db code too, allowing you to implement transactions, for example, in the future.

Don't pass ids around, pass the entities

Now that Program is acting as a barrier between the nasty, stringly-typed command line frontier and the beautiful, pure objects domain, let's make that actual domain use pure objects.

You've got these lovely types for a reason, benefit from the type checker and get extra correctness by passing the whole entity between your functions.

Recommendation

(assuming the EF model recommendation earlier)

class NoteService
{
    readonly NotesContext db;

    public NoteService(NotesContext context)
    {
        db = context;
    }

    public Note CreateNote(User user, string title, string content) => new Note { User = user, Title = title, Content = content };

    public void AddNote(Note note) => db.Notes.Add(note);

    public Note GetNote(int id) => db.Notes.Find(id);

    public void RemoveNote(Note note) => db.Notes.Remove(note);

    public void ClearNotes() => db.Notes.RemoveRange(db.Notes);
}

class UserService
{
    readonly NotesContext db;

    public UserService(NotesContext context)
    {
        db = context;
    }

    public User CreateUser(string userName) => new User() { UserName = userName };

    public void AddUser(User user) => db.Users.Add(user);

    public User GetUser(int id) => db.Users.Find(id);

    public void RemoveUser(User user) => db.Users.Remove(user);

    public void ClearUsers() => db.Users.RemoveRange(db.Users);
}


class Program
{
    private static void ExecuteAgainstDatabase(Action<NotesContext> action)
    {
        using (var db = new NotesContext())
        {
            action(db);
            db.SaveChanges();
        }
    }

    private static void ExecuteForUsers(Action<UserService> action) => ExecuteAgainstDatabase(db => action(new UserService(db)));

    private static void ExecuteForNotes(Action<NoteService> action) => ExecuteAgainstDatabase(db => action(new NoteService(db)));

    private static void AddUser(string userName) => ExecuteForUsers(us => us.AddUser(us.CreateUser(userName)));

    private static void RemoveUser(int userId) => ExecuteForUsers(us => us.RemoveUser(us.GetUser(userId)));

    private static void ClearUsers() => ExecuteForUsers(us => us.ClearUsers());

    private static void AddNote(int userId, string title, string content)
    {
        ExecuteAgainstDatabase(db =>
        {
            var noteService = new NoteService(db);
            var userService = new UserService(db);

            var user = userService.GetUser(userId);

            noteService.AddNote(noteService.CreateNote(user, title, context));
        });
    }

    private static void RemoveNote(int noteId) => ExecuteForNotes(ns => ns.RemoveNote(ns.GetNote(noteId)));

    private static void ClearNotes() => ExecuteForNotes(ns => ns.ClearNotes());
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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