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; }
}
1 Answer 1
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
.
-
\$\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– Tums2014年01月30日 06:51:36 +00:00Commented Jan 30, 2014 at 6:51
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\$