User can add and delete customers

Question

I am using asp.net core entitiy framework. The page displays the customers. The user can add new customers or delete existing ones. I am looking for ways to improve my code.

Here is my cshtml page

@Html.Partial("Index")
@model ecommerce.Models.Users
<h6>Add a new customer</h6>
<form asp-controller="Customer" asp-action="AddCustomer" method="post" role="form"> 
 <span asp-validation-for="name"></span>
 <label asp-for="name">Name</label>
 <input asp-for="name"/>
 <button type="submit">Add</button>
 <input asp-for="created_at" value="@DateTime.Now" class="time"/>
 <input asp-for="updated_at" value="@DateTime.Now" class="time"/>
</form>
@{
 if(ViewBag.NotUnique!=null)
 {
 <p>Name is alread in DB</p>
 }
 if(ViewBag.Empty!=null)
 {
 <p>Name is empty</p>
 }
 <table> 
 <tr>
 <th>Customer Name</th>
 <th>Created Date</th>
 <th>Actions</th>
 </tr>
 @if(ViewBag.Users != null)
 {
 foreach(var user in ViewBag.Users)
 {
 <tr>
 <td>@user.name</td>
 <td>@user.created_at.ToString("MMM") @user.created_at.ToString("dd") @user.created_at.ToString("yyyy")</td>
 <form action="/DeleteCustomer/@user.id" method="post">
 <td> <input type="submit" name="submit" value="Delete"/></td>
 </form> 
 </tr>
 }
 }
 </table>
}

Here is my cs file

using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Mvc;
using ecommerce.Models;
using ecommerce.Controllers;
using System.Linq;
namespace ecommerce.Controllers
{
 public class CustomerController : Controller
 {
 // GET: /Home/
 private YourContext _context;
 public CustomerController(YourContext context)
 {
 _context = context;//connection for my db
 }
 [HttpGet]
 [Route("customers")]
 public IActionResult Customers()
 {
 ViewBag.Users = _context.Users.AsEnumerable();
 return View();
 }
 [HttpPost]
 [Route("deleteCustomer/{id}")]
 public IActionResult DeleteCustomer(int id)
 {
 var temp = _context.Users.Where(x => x.id == id).SingleOrDefault();
 _context.Users.Remove(temp);
 _context.SaveChanges();
 return RedirectToAction("Customers");
 }
 [HttpPost]
 [Route("addCustomer")]
 public IActionResult AddCustomer(String name, DateTime created_at)
 {
 Users user = new Users();
 user.name = name;
 user.created_at = created_at;
 user.updated_at = created_at;
 var temp = _context.Users.AsEnumerable().ToList();
 if(temp.Any(x => x.name == name))
 {
 ViewBag.NotUnique = true;
 return RedirectToAction("Customers");
 }
 if (user.name != null)
 {
 // Success
 _context.Users.Add(user);
 _context.SaveChanges();
 return RedirectToAction("Customers");
 }
 ViewBag.Empty = true;
 // If there were errors, re-render the view but bind the model that was submitted.
 return RedirectToAction("Customers");
 }
 }
}

Answer 1

1. You don't need to enumerate twice here

   var temp = _context.Users.AsEnumerable().ToList();
   

   You can just call .ToList().

2. SingleOrDefault() also accepts predicates so the first .Where() call is unnecessary, you can just do it like this :

   Your code :

   var temp = _context.Users.Where(x => x.id == id).SingleOrDefault();
   

   Can become :

   var temp = _context.Users.SingleOrDefault(x => x.id == id);
   

3. I don't see any reason to call .ToList() here :

    var temp = _context.Users.ToList();
    if (temp.Any(x => x.name == name))
    {
    ViewBag.NotUnique = true;
    return RedirectToAction("Customers");
    }
   

   You are not performing any List<T> specific operations, (I assume Users is IEnumerable<T>), you are good with a pure IEnumerable<T> here. In fact, you don't even need the temp variable, you can just directly access _context.Users.

4. You can have just ActionResult instead of IActionResult as a return type for your methods.

Design

• Usually asp.net-mvc projects work with multiple DTO's (Data Transfer Object), which are almost identical to your actual model, but you can remove some things and add others, so that you can define the way your data will be passed around your application. Often the ViewModel is mapped in a way to it's respective DTO, to ease the creation of both objects.

  Even tho simple projects probably wont need any additional DTO's.

• Your models should have proper validation instead of some check against null in your controller, which should only verify that the validation is successful. For example DataAnnotations offers quite some variety of pre-written validation attributes. You can of course extend this with your own, but it already has the core things you might need. You can find examples here

Answer 2

You could just use

<td>@user.created_at.ToString("MMM dd yyyy")</td>

instead of

<td>@user.created_at.ToString("MMM") @user.created_at.ToString("dd") @user.created_at.ToString("yyyy")</td>