2
\$\begingroup\$

I have an admin class that controls when a user logs in and creates a new blog post. I am wondering if there is anything I can do to improve this class. Is there any way to reduce the code? Is there any repetitive code or better way of writing the functions?

public class AdminController : Controller
{
    private readonly IApplicationUserRepository _appUserRepo;
    private readonly IBlogCategoryRepository _blogCat;
    private readonly IBlogTagRepository _blogTag;
    private readonly ICategoryRepository _catRepo;
    private readonly IBlogPostRepository _postRepo;
    private readonly IBlogViewRepository _postViewRepo;
    private readonly ITagRepository _tagRepo;

    public AdminController(IBlogPostRepository postRepo, IBlogViewRepository postViewRepo, ITagRepository tagRepo, ICategoryRepository catRepo, IBlogCategoryRepository blogCat, IBlogTagRepository blogTag, IApplicationUserRepository appUserRepo)
    {
        this._postRepo = postRepo;
        this._postViewRepo = postViewRepo;
        this._tagRepo = tagRepo;
        this._catRepo = catRepo;
        this._blogCat = blogCat;
        this._blogTag = blogTag;
        this._appUserRepo = appUserRepo;
    }

    public ActionResult Setting()
    {
        string userId = IdentityExtensions.GetUserId(this.User.Identity);
        ApplicationUser currentUser = this._appUserRepo.Find(e => e.Id.Equals(userId)).FirstOrDefault();
        if (currentUser != null)
        {
            ViewBag.FirstName = currentUser.FirstName ?? " ";
            ViewBag.LastName = currentUser.LastName ?? " ";
            ViewBag.JobTitle = currentUser.JobTitle ?? " ";
            ViewBag.DateOfBirth = currentUser.DateOfBirth;
            ViewBag.Gender = currentUser.Gender;

        }
        return this.View();
    }

    [HttpPost]
    public ActionResult ChangeSetting(SettingViewModel setting)
    {
        string userId = this.User.Identity.GetUserId();
        ApplicationUser applicationUser = this._appUserRepo.Find(e => e.Id.Equals(userId)).FirstOrDefault();
        if (setting.PictureUrl != null && setting.PictureUrl.ContentLength > 0)
        {
            string fileName = Path.GetFileName(setting.PictureUrl.FileName);
            if (fileName != null)
            {
                string filename = Path.Combine(this.Server.MapPath("../Scripts/assets/images/uploads"), fileName);
                setting.PictureUrl.SaveAs(filename);
                if (applicationUser != null)
                {
                    applicationUser.PictureBigUrl = "/Scripts/assets/images/uploads/" + fileName;
                    applicationUser.PictureSmalUrl = "/Scripts/assets/images/uploads/" + fileName;
                    applicationUser.PictureUrl = "/Scripts/assets/images/uploads/" + fileName;
                }
            }
        }
        if (setting.FirstName != null && applicationUser != null)
        {
            applicationUser.FirstName = setting.FirstName;
        }

        if (setting.LastName != null)
        {
            applicationUser.LastName = setting.LastName;
        }

        if (setting.DateOfBirth != applicationUser.DateOfBirth)
        {
            applicationUser.DateOfBirth = setting.DateOfBirth;
        }
        if (setting.JobTitle != null)
        {
            applicationUser.JobTitle = setting.JobTitle;
        }
        if (setting.Gender != null)
        {
            applicationUser.Gender = setting.Gender;
        }

        this._appUserRepo.Update(applicationUser);
        return this.RedirectToAction("index", "Admin");
    }

    public ActionResult Index()
    {
        var userName = this.User.Identity.GetUserName();

        ViewBag.TotalPosts = this._postRepo.Find(e => e.UserName.Equals(userName)).Count();
        ViewBag.TotalViews = this._postViewRepo.Find(e => e.BlogPost.UserName.Equals(userName)).Count();
        ViewBag.TodayPosts = GetToday(_postRepo).ToList().FindAll(e => e.UserName.Equals(userName)).Count;
        ViewBag.TodayViews = GetToday(_postViewRepo).ToList().FindAll(e => e.BlogPost.UserName.Equals(userName)).Count;

        dynamic posts = _postRepo.GetAllQueryAble().ToList()
             .Where(e => e.UserName.Equals(userName))
             .Select(e => new
              {
                  TotalPostView = this._postViewRepo.Find(x => x.BlogPost.BlogPostId == e.BlogPostId).Count(),
                  TotalComments = 0/*e.BlogPostComments.Count(y => y.BlogPostId == e.BlogPostId)*/,
                  Status = e.BlogStatus,
                  e.DateAdded,
                  e.LastModifiedDate,
                  PostId = e.BlogPostId,
                  e.Title
              })
              .AsEnumerable()
              .Select(e => e.ToExpando()).ToList();

        ViewBag.Posts = posts;
        return this.View();
    }

    private int CalculateAge(string userName)
    {
        DateTime dateTime = DateTime.Today;
        int year1 = dateTime.Year;
        dateTime = this._appUserRepo.Find(e => e.UserName.Equals(userName)).FirstOrDefault().DateOfBirth;
        int year2 = dateTime.Year;
        return year1 - year2;
    }

    public ActionResult Editor(int id = 0)
    {
        if (id != 0)
        {

            BlogPost blogPost = this._postRepo.Find(e => e.BlogPostId.Equals(id)).FirstOrDefault();
            if (blogPost != null)
            {
                ViewBag.Content = blogPost.Content ?? "Content";

                ViewBag.BlogTitle = blogPost.Title ?? "title";
            }
        }
        else
        {
            ViewBag.Content = "Please Enter Content";
            ViewBag.BlogTitle = "Please Enter Title";
        }
        return this.View();
    }

    [ValidateInput(false)]
    public ActionResult SaveBlogPost(FormCollection formCollection)
    {
        string content = this.Request.Form["editor"];
        string title = this.Request.Form["Title"];
        string str1 = this.Request.Form["Tags"];
        string str2 = this.Request.Form["Categories"];

        this.AddItems<Tag>(str1, this._tagRepo);
        this.AddItems<Category>(str2, this._catRepo);
        this.CreateBlogPost(content, title);
        BlogPost blogPost = this._postRepo.GetAll().LastOrDefault();
        if (blogPost != null)
        {
            int blogPostId = blogPost.BlogPostId;
            this.CreateBlogTagIfNotExist(str1, blogPostId);
            this.CreateBlogCategoryIfNotExist(str2, blogPostId);
        }
        return this.RedirectToAction("index", "Admin");
    }

    [ValidateInput(false)]
    public ActionResult UpdateBlogPost(FormCollection formCollection)
    {
        string content = this.Request.Form["editor"];
        string title = this.Request.Form["Title"];
        string str1 = this.Request.Form["Tags"];
        string str2 = this.Request.Form["Categories"];

        this.AddItems<Tag>(str1, this._tagRepo);
        this.AddItems<Category>(str2, this._catRepo);
        this.CreateBlogPost(content, title);
        BlogPost blogPost = this._postRepo.GetAll().LastOrDefault();
        if (blogPost != null)
        {
            int blogPostId = blogPost.BlogPostId;
            this.CreateBlogTagIfNotExist(str1, blogPostId);
            this.CreateBlogCategoryIfNotExist(str2, blogPostId);
        }
        return this.RedirectToAction("index", "Admin");
    }


    private void CreateBlogPost(string content, string title)
    {
        this._postRepo.Add(new BlogPost
        {
            Content = content,
            IsDeleted = new bool?(false),
            UserName = this.User.Identity.GetUserName(),
            DateAdded = DateTime.Now,
            Title = title,
            LastModifiedDate = DateTime.Now
        });
    }

    private void CreateBlogTagIfNotExist(string tags, int lastInsertedId)
    {
        using (List<Tag>.Enumerator enumerator = this.GetItems<Tag>(tags, this._tagRepo).GetEnumerator())
        {
            while (enumerator.MoveNext())
            {
                _blogTag.Add(new BlogTag
                {
                    TagId = enumerator.Current.TagId,
                    BlogPostId = lastInsertedId,
                    DateAdded = DateTime.Now,
                    DateDeleted = new DateTime(2000,1,1),
                    LastModifiedDate = DateTime.Now
                }
              );
            }
        }
    }

    private void CreateBlogCategoryIfNotExist(string category, int lastInsertedId)
    {
        using (List<Category>.Enumerator enumerator = this.GetItems<Category>(category, this._catRepo).GetEnumerator())
        {
            while (enumerator.MoveNext())
            {
                if (enumerator.Current != null)
                    this._blogCat.Add(new BlogCategory
                    {
                        BlogPostId = lastInsertedId,
                        CategoryId = enumerator.Current.CategoryId,
                        DateAdded = DateTime.Now,
                        LastModifiedDate = DateTime.Now,
                        IsDeleted = (new bool?(false)),
                        DateDeleted = new DateTime(2000, 1, 1)
                    });
            }
        }
    }

    public List<T> GetItems<T>(string tags, IAddTagOrCategory<T> repository) where T : class, IHasName
    {
        string[] strArray = tags.Split(',');
        return strArray.Select(str => repository.Find(e => e.Name.Equals(str)).FirstOrDefault()).ToList();
    }

    public void AddItems<T>(string items, IAddTagOrCategory<T> repository) where T : class, IHasName, new()
    {

        var allTags = repository.GetAllQueryAble();
        string[] tag = items.Split(',');

        foreach (var item in tag)
        {
            if (!allTags.Any(e => e.Name.Contains(item)))
            {
                repository.AddTagOrCategory
                (
                  item,
                   DateTime.Now,
                   DateTime.Now,
                     false

                );
            }
        }
    }

    private IEnumerable<T> GetToday<T>(IRepository<T> repository) where T : class, IHazDateAdded
    {
        return MenuExtensions.GetToday<T>(repository.GetAllQueryAble());
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ In your ChangeSetting() method you are not checking enough for applicationUser != null. Assume setting.LastName != null but applicationUser==null you will get an NullReferenceException. For the case of applicationUser==null you should redirect to an error page. \$\endgroup\$
    – Heslacher
    Sep 15, 2014 at 5:52

1 Answer 1

4
\$\begingroup\$
  1. The UpdateBlogPost and SaveBlogPost methods are identical (unless I missed something). Not sure if this is a bug or intended behaviour but if it is intended then this should be extracted into a separate method so it can be called from both actions.

  2. From my experience it is very rarely required to use the enumerator interface directly, like you do in your CreateTag/CategoryIfNotExist methods. Those can be rewritten to:

    foreach (var blogCategory in GetItems<Category>(category, _catRepo))
    {
        blogCategory.Add(new BlogCategory { ... });
    }
    

    It's more concise and and follows the expected usage pattern.

  3. In your AddItems method you do if (!allTags.Any(e => e.Name.Contains(item))) - this means you will not add a tag if it's a substring of an already existing one. So for Example if you write some blog posts about VB.NET then you cannot add a separate .NET tag.

  4. You sometimes use Tag and sometimes Item for the same thing - chose one and stick to a consistent vocabulary, otherwise it becomes confusing in the long run.

  5. There are a bunch of hard-coded strings in there, some for paths some for resources. Those should be moved into config sections or string resources.

  6. I have not much experience with ASP.NET MVC but in general the gist is: Fat models and skinny controllers. And your controller is quite fat - so consider moving the logic out of it into separate entities.

    Moving the code out of there will make it easier to adhere to the single responsibility principle - the responsibility of your controller is delegating the work to other entities which is easy to test and your business logic entities should just be performing one single thing and therefore be easy to test as well. The way your controller is right now it looks like the unit tests for it will be fairly involved.

    Another reason for that might be that your controller is generally tied to some framework because it inherits from some base class. So if you have a lot of logic in there then it will be harder to move to a different framework in the future (even though in the real world that is usually not a big concern because going to a different framework rarely happens)

  7. You use FirstOrDefault a lot - I'm somewhat dubious about the usefulness of it. If there is more than one entity would this be a bug? Would it cause potential data corruption or maybe leak data? I'd consider using SingleOrDefault and let it throw (or maybe wrap into a custom exception to indicate that something has gone wrong) when more than one item exists - it's better to detect problems early. Sure it might be inconvenient for the user that something doesn't work but that's better than to silently corrupt data or create lot's of garbage in the background.

\$\endgroup\$
2
  • \$\begingroup\$ For point 7, do you mean Single (or SingleOrDefault) rather than First? \$\endgroup\$ Sep 14, 2014 at 21:05
  • \$\begingroup\$ @ChrisWue thank for the well detail answer. I am currently making some changes to my class based on the suggestion. \$\endgroup\$ Sep 19, 2014 at 7:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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