1
\$\begingroup\$

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
            });
        }
    }
\$\endgroup\$
4
\$\begingroup\$

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):

  1. Are allTags and allCategories actually required as I can't see where they are used? If not, then I would suggest removing them to reduce clutter.
  2. I would consider moving the two loops to their own method to further increase the SRP at a function level.
  3. I assume blogTag and blogCat 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
            })); 
}
\$\endgroup\$
  • \$\begingroup\$ thankz for the example code. Good eye on noticing the two variables not been use. I end up with similar code. But what does this.blogCat.Clear() do ? \$\endgroup\$ – DacorieS Aug 4 '14 at 16:44
  • 1
    \$\begingroup\$ +1 for suggesting prefixing instance variable names. Doing so goes against Microsoft's .NET naming guidelines (the original ones, anyway), but you see it a lot in sample code from MS anyway, and I've always done it. It just sucks to see variables that look like local variables, but aren't, so you waste time looking for the declarations, etc. Ugh. \$\endgroup\$ – Craig Aug 4 '14 at 17:14
  • \$\begingroup\$ @Craig this is so true. I realize it is easier to identify local variables from instance variables. \$\endgroup\$ – DacorieS Aug 4 '14 at 17:55
  • \$\begingroup\$ @donquixtoDac I wasn't sure if you would want to clear the blog lists before adding to them. If not, then you could safely removing this line. \$\endgroup\$ – dreza Aug 4 '14 at 20:24
5
\$\begingroup\$
  • Parameters should be camelCased, so Tags should be tags. As far as I can tell though only one tag is passed in, so it should be the singular tag.
  • 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.
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Good advice @Mark. I will look into it thanx \$\endgroup\$ – DacorieS Aug 4 '14 at 16:47
1
\$\begingroup\$
  • 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?
\$\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.