7
\$\begingroup\$

So we had an argument whether this question is on topic: RESTful movie shop

I see this question as off-topic although it seems to satisfy most rules.

There's code. It works. It's real...

But the questions asked by OP does not seem to look for a code review.

He asks if its ok to use verbs in RESTful API and similar things. If you replace all the code snippets with descriptions of HTTP requests, query parameters, response statuses, etc. all the text of the post is still valid, because it never actually talks about the code. It just uses the code to show us how the HTTP API is designed and asks if the design follows best practices.

To me it seems that OP created a design, implemented it, then stopped and thought maybe he should have the design reviewed first and he used the code to describe the design to potential reviewers because that's what he already had.

A reference to rule that says it may be off-topic is here in the help/dont-ask:

Higher-level architecture and design of software systems

We review code, not concepts, diagrams, or outlines. Whiteboard-style questions may be suitable on Software Engineering if they are specific.

But it's not clear whether the presence of code makes it on-topic or not.

So how is it?

I am including copy of the post here too to keep the exact wording at the time and have it self-contained:


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?


And comments:

Questions about best practices are outside the scope of this site. – slepic

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. – Peilonrayz

@Peilonrayz yes, the code looks good for a code review, but the OP doesn't seem to be interested as much in code review as he is interested in a design review and best practices with RESTful APIs. That makes it off topic. A good code review would probably provide answers to OP's questions as well, but it shouldn't be the main focus. As it Is now, it seems to fit better on Software Engineering Stack Exchange. Unfortunately, there is no such migration path. – slepic

@slepic Please can you provide links to Meta that show that this is indeed off-topic. – Peilonrayz

@Peilonrayz section "Higher-level architecture and design of software systems" of codereview.stackexchange.com/help/dont-ask – slepic

@slepic That's if there's no code. And there is clearly code. – Peilonrayz

I understand it Is subject to interpretation. I lean more towards off-topic, but I understand it may be viewed differently. Nevertheless, I believe SE has a better potential to satisfy the OP And He should go ask there for his own sake regardless of this question being closed or not. – slepic

If I post a code snippet and then talk about my holiday, am I asking for code review? – slepic

@slepic It doesn't matter if SE exists or not. The only thing that makes a question off-topic on Code Review is Code Review's rules. Additionally your simile has no likeness to the current situation and only unjustly paints the question in a bad light. Please take this to meta if you have a problem. – Peilonrayz

| |
\$\endgroup\$
  • 4
    \$\begingroup\$ "But it's not clear whether the presence of code makes it on-topic or not." That's what it's all about, the code. And that's what we can review, the code. If it turns out OP is more interested in the design than the code, they're out of luck. But I can't tell if that's going to be the case here. It's not a question I would personally review, but that doesn't mean it's violating the site's scope. \$\endgroup\$ – Mast Jun 18 at 17:41
11
\$\begingroup\$

Existing rules

These are covered in multiple questions

Question body:

Reviewing design:

If there is little/no code:

Specific design questions:

Ok, that's a lot of links but you should notice a general trend between them.

  1. We need to know what your code does from the description.
  2. Design questions without context (code) are off-topic.
  3. Many questions about design are ok.

Topicality of the question

  • If you replace all the code snippets with descriptions of HTTP requests, query params, response statuses, etc. all the text of the post is still valid, because it never actually talks about the code.

    Yes, if you remove the code it would be off-topic.
    But through their description I know what their code is doing.

  • It just uses the code to show us how the http api is designed and asks if the design follows best practices.

    No, the user has written some code and now has come to us for a review.

  • A good code review would probably provide answers to OP's questions as well, but it shouldn't be the main focus. As it Is now, it seems to fit better on Software Engineering Stack Exchange. Unfortunately, there is no such migration path.

    You're on the right track, but you got the wrong conclusion. Because the question doesn't violate the "any or all" rule then there is no problem here.

    Do I want feedback about any or all facets of the code?

    Feel free to call attention to specific areas you are concerned about (performance, formatting, etc). However, any aspect of the code posted is fair game for feedback and criticism.

    If you remove the code then it could make a good question on Software Engineering but why does that matter when it's not off-topic here?

    If you wish to recommend another site that has some overlap with ours, then you can recommend the OP also uses that site. But, don't recommend cross-posting on the Stack Exchange network. And the presence of another site doesn't affect our rules.

  • To me it seems that OP created a design, implemented it, then stopped and thought maybe he should have the design reviewed first and he used the code to describe the design to potential reviewrs because that's what he already had.

    Yes that's probably what happened. I've posted questions like that too. My first design is never any good, and I want people to rip it to shreds.
    But again, you've come at this the wrong way. Just because Software Engineering exists doesn't make this question off-topic. And since they've got working code it's a perfect fit for Code Review.

  • yes, the code looks good for a code review, but the OP doesn't seem to be interested as much in code review as he is interested in a design review and best practices with RESTful APIs. That makes it off topic.

    I believe Donald.McLean explained this perfectly.

    There is one school of thought that says "the code is the design".

  • A reference to rule that sais it may be off-topic is here:

    Higher-level architecture and design of software systems

    We review code, not concepts, diagrams, or outlines. Whiteboard-style questions may be suitable on Software Engineering if they are specific.

    This again falls inline with all the above. This rule is only if there is no code.

  • If I post a code snippet and then talk about my holiday, am I asking for code review?

    Yes. But we'll remove the information about your holiday. If you start an edit war then we'll have to escalate to the mods.

So tying this all together. They have code. They dislike a certain part of their code. They want their code to be better.
How isn't this on-topic for Code Review?

| |
\$\endgroup\$
  • 4
    \$\begingroup\$ @slepic: Thanks for your interest in helping to maintain the high quality of CodeReview! I understand your concern, but agree the particular question is on topic for the reasons mentioned here. \$\endgroup\$ – Edward Jun 18 at 17:40

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .