4
\$\begingroup\$

Is the following good design for doing entity framework code first? What am I missing for a production system? I haven't included all my code, just a snapshot...

My application, doesn't update the database; it just reads it.

public class Document
{
    [Key, ScaffoldColumn(false)]
    public Int32 DocumentID { get; set; }

    [ScaffoldColumn(false)]
    public Boolean? Status { get; set; }

    [Display(Name = "Document Text"), DataType(DataType.MultilineText)]
    public String DocumentText { get; set; }

    [ScaffoldColumn(false), DataType(DataType.Url)]
    public String DocumentFolderPath { get; set; }

    [ScaffoldColumn(false), DataType(DataType.Url)]
    public String DocumentJSONPath { get; set; }

    [ForeignKey("DocumentID")]
    public virtual DocumentMetadata Metadata { get; set; }

    [ForeignKey("DocumentID")]
    public virtual ICollection<Page> Pages { get; set; }
}

public class Page
{
    [Key, ScaffoldColumn(false)]
    public Int32 PageID { get; set; }

    [Required, Display(Name = "Page"), DataType(DataType.Text)]
    public Int32 PageNumber { get; set; }

    [Required, ScaffoldColumn(false)]
    public Int32 DocumentID { get; set; }

    [ScaffoldColumn(false), DataType(DataType.ImageUrl)]
    public String ImagePath { get; set; }

    [Required, Display(Name = "Page Text"), DataType(DataType.MultilineText)]
    public String PageText { get; set; }

    [NotMapped]
    public String HighlightedText { get; set; }

    [ForeignKey("PageID")]
    public virtual ICollection<Word> Words { get; set; }
}

public class ArchiveDatabaseInitializer : DropCreateDatabaseIfModelChanges<ArchiveContext> 
{
    protected override void Seed(ArchiveContext context)
    {
        try
        {
            GetDocuments().ForEach(d => context.Documents.Add(d));
            GetDocumentMetatdata().ForEach(d => context.DocMetadata.Add(d));
            GetPages().ForEach(d => context.Pages.Add(d));
            GetWords().ForEach(d => context.Words.Add(d));
            context.SaveChanges();
        }
        catch (Exception ex)
        {
            throw ex;
        }
    }

    private static List<Document> GetDocuments()
    {
        var documents = new List<Document> {
            new Document
            {
                DocumentID = 1,
                DocumentFolderPath = @"Doc1",
                DocumentJSONPath = @"Doc1.json",
                Status = true
            },
            new Document
            {
                DocumentID = 2,
                DocumentFolderPath = @"Doc2",
                DocumentJSONPath = @"Doc2.json",
                Status = true
            }
        };
        return documents;
    }

public class ArchiveContext : DbContext
{
    public ArchiveContext()
        : base("archiveDB")
    {
    }
    public DbSet<Document> Documents { get; set; }
    public DbSet<DocumentMetadata> DocMetadata { get; set; }
    public DbSet<Page> Pages { get; set; }
    public DbSet<Word> Words { get; set; }
}
\$\endgroup\$
  • \$\begingroup\$ What makes you think you're missing anything for a production system? \$\endgroup\$ – Mathieu Guindon Jan 29 '14 at 17:52
  • \$\begingroup\$ This is the first time I've used EF so I don't know what I don't know. ;) \$\endgroup\$ – Tums Jan 29 '14 at 17:54
  • \$\begingroup\$ You got it working? \$\endgroup\$ – Mathieu Guindon Jan 29 '14 at 18:00
  • \$\begingroup\$ I don't think the ICollection<Page> navigation property can have a FK attribute AND work. But I could be wrong. It just... looks weird to me. I've favorited this question, I'll get back to it when I have a chance :) \$\endgroup\$ – Mathieu Guindon Jan 29 '14 at 18:38
  • 1
    \$\begingroup\$ Yes it is working. It looks like it gets the data as expected. I just want to know if this is the correct way of doing this. \$\endgroup\$ – Tums Jan 29 '14 at 20:33
4
\$\begingroup\$

If you chose the code-first approach, I presume you don't have to deal with the constraints of an existing database. If that assumption is correct, then I'd say you've done it the hard way.

Convention Over Configuration

Entity Framework can infer keys (primary and foreign) from the names of your entity members; your code isn't leveraging this formidable capability.

For a greenfield project, I like to start with a base entity type:

public abstract class EntityBase
{
    public int Id { get; set; } // inferred PK
    public DateTime DateInserted { get; set; }
    public DateTime? DateUpdated { get; set; }
}

Now I can derive, say, Document from that class:

public class Document : EntityBase
{
    public bool? Status { get; set; }
    public string DocumentText { get; set; }
    // ...

    public virtual DocumentMetadata Metadata { get; set; } // inferred FK
}

The PK is inferred from the inherited Id property (or [TypeName]Id), and the mere mention of a virtual property referencing another entity type is enough for EF to understand you want a FK there.

I'll pause here to mention that I was somewhat thrown off by your non-usage of the C# language aliases for System.String, System.Int32 and System.Boolean. They're typically written as string, int and bool.


You mention you're only reading from the database. This sounds like you're going code-first with an existing database. It's certainly possible, but the best way to do this in my opinion, is to reverse-engineer the database into entity classes - then you get the best of both worlds, and the generated code can teach you a lot about how EF works.


I think the presence of DisplayAttribute and NotMappedAttribute smells like you're possibly using your entities as both data entities and presentation / ViewModels. I think I would separate these concerns and create separate classes for presentation purposes, where HighlightedText means something relevant.

Also note that relying on a declarative DisplayAttribute will make it quite a pain to localize the application, if you ever need to do that in the future; you'll want these things resolved at run-time, accounting for Thread.CurrentThread.CurrentUICulture.


The naming is overall ok, except I don't like the Status column. It's just too vague of a name. What do the null, true and false values mean? The name should convey that meaning. Also I wouldn't prefix most of Document's members with the word "Document", and ID should follow the PascalCasing convention and be Id.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the detailed response. I will be updating my code with your suggestions. About the FK being explicitly set, when I didn't have it there, the database did not create a foreign key in the database. It may be because it wasn't recognizing my id's as keys. I will try it and find out. \$\endgroup\$ – Tums Jan 30 '14 at 6:51

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.