2
\$\begingroup\$

Just would like to ask if these custom web API naming follow best practice? Below are some of the web API methods I have created for my pet project using ASP.NET .NET Framework 4.7.

I understand that the standard CRUD naming convention especially when I use the scaffolding option. For example, this Cart controller web API, the scaffolded code are.

   // GET: api/Carts
    public IQueryable<Cart> GetCarts()
    {
        return db.Carts;
    }

    // GET: api/Carts/5
    [ResponseType(typeof(Cart))]
    public async Task<IHttpActionResult> GetCart(int id)
    {
        Cart cart = await db.Carts.FindAsync(id);
        if (cart == null)
        {
            return NotFound();
        }

        return Ok(cart);
    }

    // PUT: api/Carts/5
    [ResponseType(typeof(void))]
    public async Task<IHttpActionResult> PutCart(int id, Cart cart)
    {
        if (!ModelState.IsValid)
        {
            return BadRequest(ModelState);
        }

        if (id != cart.Id)
        {
            return BadRequest();
        }

        db.Entry(cart).State = EntityState.Modified;

        try
        {
            await db.SaveChangesAsync();
        }
        catch (DbUpdateConcurrencyException)
        {
            if (!CartExists(id))
            {
                return NotFound();
            }
            else
            {
                throw;
            }
        }

        return StatusCode(HttpStatusCode.NoContent);
    }

    // POST: api/Carts
    [ResponseType(typeof(Cart))]
    public async Task<IHttpActionResult> PostCart(Cart cart)
    {
        if (!ModelState.IsValid)
        {
            return BadRequest(ModelState);
        }            
        
        db.Carts.Add(cart);
        await db.SaveChangesAsync();
        
        return CreatedAtRoute("DefaultApi", new { id = cart.Id }, cart);
    }

    // DELETE: api/Carts/5
    [ResponseType(typeof(Cart))]
    public async Task<IHttpActionResult> DeleteCart(int id)
    {
        Cart cart = await db.Carts.FindAsync(id);
        if (cart == null)
        {
            return NotFound();
        }

        db.Carts.Remove(cart);
        await db.SaveChangesAsync();

        return Ok(cart);
    }

But, I don't use much of those CRUD method above. So, I added my own like below:

    [Route("api/Carts/GetClearAllCart/{id:int}")]
    public async Task<IHttpActionResult> GetClearAllCart(int id)
    {
        var carts = db.Carts.Where(c => c.UsersId == id);
        if (carts == null)
        {
            return NotFound();
        }

        db.Carts.RemoveRange(carts);
        await db.SaveChangesAsync();

        return Ok(carts);
    }

    [Route("api/Carts/GetClearOne/{id:int}")]
    public async Task<IHttpActionResult> GetClearOneItemFromCart(int id)
    {
        var cart = db.Carts.SingleOrDefault(c => c.SeatId == id);
        if (cart == null)
        {
            return NotFound();
        }

        db.Carts.Remove(cart);
        await db.SaveChangesAsync();

        return Ok();
    }

    [Route("api/Carts/GetClearAllByUserId/{id:int}")]
    public async Task<IHttpActionResult> GetClearAllByUserId(int id)
    {
        var cart = db.Carts.Where(c => c.UsersId == id);
        if (cart == null)
        {
            return NotFound();
        }

        db.Carts.RemoveRange(cart);
        await db.SaveChangesAsync();

        return Ok();
    }

    [Route("api/Carts/GetCartsByUserId/{id:int}")]
    public IQueryable<CartVM> GetCartsByUserId(int id)
    {
        var cartVMList = new List<CartVM>();
        var selectedCart = db.Carts.Where(c => c.UsersId == id);
        foreach (var item in selectedCart)
        {
            var cartVM = new CartVM();
            cartVM.MovieTitle = item.MovieTitle;
            cartVM.HallNo = item.HallNo;
            cartVM.MovieDateTime = item.MovieDateTime;
            cartVM.Seats = item.Seats;
            cartVM.SeatId = item.SeatId;
            cartVMList.Add(cartVM);
        }
        return cartVMList.AsQueryable();
    }

 // GET: api/Movies
    public IQueryable<MovieVM> GetMovies()
    {
        
        var movieVMList = new List<MovieVM>();

        foreach (var item in db.Movies)
        {
            var movieVM = new MovieVM();
            movieVM.MovieTitle = item.MovieTitle;
            movieVM.Id = item.Id;
            movieVM.PhotoFile = item.PhotoFile;
            movieVMList.Add(movieVM);
        }

        return movieVMList.AsQueryable();
    }

    // GET: api/Movies/5
    [ResponseType(typeof(Movie))]
    public async Task<IHttpActionResult> GetMovie(int id)
    {
        Movie movie = await db.Movies.FindAsync(id);
        if (movie == null)
        {
            return NotFound();
        }

        return Ok(movie);
    }

I read that we should focus on the noun for RESTful API instead of verbs which is SOAP. How can we create a custom web API only with noun and without verbs? Any comment on my custom web API methods?

\$\endgroup\$
  • 3
    \$\begingroup\$ This looks on-topic to me IDK why people are saying it's not (They don't have 3k yet so there's no close votes either). Your question (description) seems somewhat high level, but your code looks fairly solid. \$\endgroup\$ – Peilonrayz Jun 18 at 15:00
  • 3
    \$\begingroup\$ @Peilonrayz alright, there codereview.meta.stackexchange.com/questions/10500/… \$\endgroup\$ – slepic Jun 18 at 16:24
  • 1
    \$\begingroup\$ Can you tell us more about the goal of your API? What prompted you to write this code? \$\endgroup\$ – Mast Jun 18 at 17:46
  • 3
    \$\begingroup\$ @SteveNgai I don't know if you've been watching the comments and the following discussion on meta. It seems your question is indeed ontopic. I've already retracted my close vote. Nevertheless, chances are that your question is more suitable for Software Engineering Stack Exchange, if you are interested in design review rather then code review, which seems to be the case, at least to me. If you decide to go ask there, please delete the post here, so that the answers are not fragmented over multiple sites. That is something I wanted to make clear, because I did not when mentioned it first time. \$\endgroup\$ – slepic Jun 19 at 4:30
  • 2
    \$\begingroup\$ "GetClearAllCart", "GetClearOne" etc. are not proper English and kinda break my brain. \$\endgroup\$ – BCdotWEB Jun 19 at 7:50
1
\$\begingroup\$

The only thing that stands out to me is these blocks:

        var cartVM = new CartVM();
        cartVM.MovieTitle = item.MovieTitle;
        cartVM.HallNo = item.HallNo;
        cartVM.MovieDateTime = item.MovieDateTime;
        cartVM.Seats = item.Seats;
        cartVM.SeatId = item.SeatId;

        var movieVM = new MovieVM();
        movieVM.MovieTitle = item.MovieTitle;
        movieVM.Id = item.Id;
        movieVM.PhotoFile = item.PhotoFile;

which should use inline property assignment, i.e.

        var cartVM = new CartVM() {
            MovieTitle = item.MovieTitle,
            HallNo = item.HallNo,
            MovieDateTime = item.MovieDateTime,
            Seats = item.Seats,
            SeatId = item.SeatId
        };

        var movieVM = new MovieVM() {
            MovieTitle = item.MovieTitle,
            Id = item.Id,
            PhotoFile = item.PhotoFile
        };

Better yet would be a constructor on each of those VM classes that accepts your item.

| improve this answer | |
\$\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.