patterncsharpMinor
User can add and delete customers
Viewed 0 times
candeleteuserandcustomersadd
Problem
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
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 = _c
Here is my cshtml page
@Html.Partial("Index")
@model ecommerce.Models.Users
Add a new customer
Name
Add
@{
if(ViewBag.NotUnique!=null)
{
Name is alread in DB
}
if(ViewBag.Empty!=null)
{
Name is empty
}
Customer Name
Created Date
Actions
@if(ViewBag.Users != null)
{
foreach(var user in ViewBag.Users)
{
@user.name
@user.created_at.ToString("MMM") @user.created_at.ToString("dd") @user.created_at.ToString("yyyy")
}
}
}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 = _c
Solution
-
You don't need to enumerate twice here
You can just call
-
Your code :
Can become :
-
I don't see any reason to call
You are not performing any
-
You can have just
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
You don't need to enumerate twice here
var temp = _context.Users.AsEnumerable().ToList();You can just call
.ToList().-
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);-
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 specific operations, (I assume Users is IEnumerable), you are good with a pure IEnumerable here. In fact, you don't even need the temp variable, you can just directly access _context.Users.-
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 hereCode Snippets
var temp = _context.Users.AsEnumerable().ToList();var temp = _context.Users.Where(x => x.id == id).SingleOrDefault();var temp = _context.Users.SingleOrDefault(x => x.id == id);var temp = _context.Users.ToList();
if (temp.Any(x => x.name == name))
{
ViewBag.NotUnique = true;
return RedirectToAction("Customers");
}Context
StackExchange Code Review Q#152515, answer score: 6
Revisions (0)
No revisions yet.