my fellow proggies. Recently I've started learning c#/.net APIs,so I'm kinda new to this. I made a simple API project with controllers by myself/tutorial.everything works but when my friend saw it he sad it was a total mess/non readable but didn't elaborate further or explaining why. Here's the controller.cs file and I'll be happy with any kind of advice/suggestion about this topic.

using System.ComponentModel;
using DragonAge.Data;
using DragonAge.Models;
using DragonAge.Services;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.DependencyInjection;


namespace DragonAge.Controllers;

[ApiController]
[Route("[controller]")]
public class CodicesController : ControllerBase
{
private readonly DatabaseContext _db;
    private readonly CodicesService _service;

    public CodicesController(DatabaseContext databaseContext)
    {
        _service = new CodicesService(databaseContext);
        _db = databaseContext;
    }
    [HttpGet]
    [Route("GetAll")]
    public ActionResult<List<Codex>> ReadAll() => _service.ReadAll();

    [HttpGet]
    [Route("{id:int}")]
        
    public ActionResult<Codex> GetById(int id) =>  _db.Codices.Single(codex => codex.Id == id);


    [HttpGet]
    [Route(("GetByCategory"))]

    public ActionResult<List<Codex>> GetByCategory(string category) => _db.Codices.Where(codex => codex.Category == category).ToList();

    [HttpPost]
    [Route("Create")]
    public async Task<ActionResult<List<Codex>>> CreateNewGame(Codex codex)
    {
        _db.Codices.Add(codex);
        await _db.SaveChangesAsync();
        return StatusCode(201);
    [HttpPatch]
    [Route("Edit")]
    public async Task<ActionResult<List<Codex>>> EditGame(int id, Codex codex)
    {
        var gameToEdit = _db.Codices.FirstOrDefault(x => x.Id == id);
        if (gameToEdit == null) return Ok();
        gameToEdit.Category = codex.Category;
        gameToEdit.Contents = codex.Contents;
        gameToEdit.Summary = codex.Summary;
        gameToEdit.Title = codex.Title;
        await _db.SaveChangesAsync();
        return StatusCode(201);
    [HttpDelete]
    [Route("Deleteaz")]
    public async Task<ActionResult<List<Codex>>> DeleteGameById(int id, Codex codex)
    {
       _db.Codices.Remove(_db.Codices.Single(x => x.Id == id));
        await _db.SaveChangesAsync();
        return StatusCode(201);
    }
}

CodePudding user response：

1. using lambda expression for methods sometimes can make a mess on your codes. lambda expression is sth => sth Instead of that you can use brackets, like your last function(DeleteGameById).

2. Somewhere in your code, brackets didn't close and this can make error in your project.

3. Typing enter key and have space after and before a function can clean your code.

4. If attributes of a function(e.g [HttpPost]) stick with them function and each other, code will be cleaner.

CodePudding user response：

My top suggestion would be to use the repository pattern so that you can write unit tests for the controller which aren't dependent on the database.

Advantages of Repository Design Pattern

• Testing controllers becomes easy because the testing framework need not run against the actual database access code.
• Repository Design Pattern separates the actual database, queries and other data access logic from the rest of the application.
• Business logic can access the data object without having knowledge of the underlying data access architecture.
• Business logic is not aware of whether the application is using LINQ to SQL or ADO.NET. In the future, underlying data sources or architecture can be changed without affecting the business logic.
• Caching strategy for the data source can be centralized.
• Centralizing the data access logic, so code maintainability is easier

It looks like you're already doing this to some extent with the CodicesService class, so extend that class to handle all your data access.

1. Anywhere in the controller that you reference _db.Codices or _db.SaveChangesAsync, move that logic into a method in the CodicesService class and call that method from the controller.
2. Create an ICodicesService interface which contains the method signatures for the public methods in CodicesService.
3. Make CodicesService implement ICodicesService, i.e. public class CodicesService : ICodicesService.
4. Change the controller's constructor so that it accepts an ICodicesService instance as a parameter, and change the data type of _service from CodicesService to ICodicesService.
5. Use dependency injection to tell ASP.net that when a controller needs an ICodicesService, a CodicesService should be used

If you don't feel ready to tackle dependency injection for now then you can set _service to a new instance of CodicesService in the controller's constructor, i.e. _service = new CodicesService(_db);, instead of setting it from a constructor parameter. However, you won't unlock the benefits of unit testability until you've implemented dependency injection. If you're using ASP.net full framework rather than ASP.net core then consider switching to ASP.net core, which has built-in dependency injection capabilities.