Problem
The code below is used to check if there are tags or categories in the database. If they exist, they are not added to the database, otherwise they are added.
Is this code efficient and easy to read?
private void CreateTagCategoryBlog(string Tags, string category)
{
int lastInsertedId = postRepo.GetAll().LastOrDefault().BlogPostId;
var allTags = tagRepo.GetAll().Where(e => e.Name.Contains(Tags)).ToList().DefaultIfEmpty();
var allCategories = catRepo.GetAll().Where(e => e.Name.Contains(category)).ToList().DefaultIfEmpty();
foreach (var x in GetItems<Tag>(Tags, tagRepo))
{
blogTag.Add(new BlogTag
{
TagId = x.TagID,
BlogPostId = lastInsertedId,
DateAdded = DateTime.Now,
LastModifiedDate = DateTime.Now,
IsDeleted = false
});
}
foreach (var x in GetItems<Category>(category, catRepo))
{
blogCat.Add(new BlogCategory
{
BlogPostId = lastInsertedId,
CategoryId = x.CategoryId,
DateAdded = DateTime.Now,
LastModifiedDate = DateTime.Now,
IsDeleted = false
});
}
}
Solution
Is this code efficient and easy to read?
Yes, it’s easy enough to read, although there is not much there to read so that helps. I do have a couple of additions (in addition to @ckuhn203 comments):
- Are
allTags
andallCategories
actually required as I can’t see where they are used? If not, then I would suggest removing them to reduce clutter. - I would consider moving the two loops to their own method to further increase the SRP at a function level.
- I assume
blogTag
andblogCat
are instance variables? If so, I would consider prefixing all class instance variables with this to more easily differentiate them from local variables.
Refactored code might look like:
private void CreateTagCategoryBlog(string tags, string category)
{
int lastInsertedId = this.postRepo.GetAll().LastOrDefault().BlogPostId;
CreateTags(tags, lastInsertedId);
CreateCategories(category, lastInsertedId);
}
private void CreateTags(string tags, int lastInsertedId)
{
this.blogTag.Clear();
var tags = GetItems<Tag>(tags, this.tagRepo);
this.blogTag.AddRange(tags.Select(x => new BlogTag
{
TagId = x.TagID,
BlogPostId = lastInsertedId,
DateAdded = DateTime.Now,
LastModifiedDate = DateTime.Now,
IsDeleted = false
}));
}
private void CreateCategories(string category, int lastInsertedId)
{
this.blogCat.Clear();
var categories = GetItems<Category>(category, this.catRepo);
this.blogCat.AddRange(categories.Select(x => new BlogCategory
{
BlogPostId = lastInsertedId,
CategoryId = x.CategoryId,
DateAdded = DateTime.Now,
LastModifiedDate = DateTime.Now,
IsDeleted = false
}));
}
- Parameters should be camelCased, so
Tags
should betags
. As far as I can tell though only one tag is passed in, so it should be the singulartag
. x
isn’t a very good variable name. It looks like you used it as a generic name because you were copy/pasting. The two loops are slightly different, but not by a whole lot. Consider how you could possibly pass the string and repository into a method instead.
Could be going completely crazy here but not sure getting the lastInsertedId
for the blog post will always give you the blog post you want to be adding tags/categories to. You could potentially be adding the tags/categories to the wrong blog post.
I would definitely pass in the BlogPostId
to the method rather than relying on the last inserted record to be the one I want.
- Your function does more than one things. Break it further one for tags and one for categories.
- Choose wiser function names. e.g. CreateTagIfNotExists
- Where allTags and allCategories variables used?