2
\$\begingroup\$

I'm learning ASP.NET and I've created a few simple APIs, so I can consider myself maybe an intermediate beginner.

I've decided to create a tutorial serving two purposes: to solidify what I've learned and to fulfil a demand for beginner tutorials in my language.

I just created a very simple API with CRUD functionalities and I'd like you to review it. I've purposefully not used Repository Pattern and DTOs because they can be confusing for someone that's just getting started (at least that was my experience). But I do plan to add those on the next phase, along with validation and error handling.

Please tear everything apart, I need all possible feedback as I need to understand every line of the code I write.

Controller:

    [ApiController]
    [Route("api/records")]
    public class RecordsController : ControllerBase
    {
        private readonly DataContext _context;

        public RecordsController(DataContext context)
        {
            _context = context;
        }
        
        [HttpGet]
        public ActionResult<IEnumerable<Record>> GetRecords()
        {
            return _context.Records.ToList();
        }

        [HttpGet("{id}")]
        public ActionResult<Record> GetRecordById(int id)
        {
            return _context.Records.Find(id);
        }

        [HttpPost]
        public ActionResult<Record> AddRecord([FromBody] Record record)
        {
            _context.Records.Add(record);
            _context.SaveChanges();

            return Ok();
        }

        [HttpDelete("{id}")]
        public ActionResult<Record> DeleteRecord(int id)
        {
            var recordForDeletion = _context.Records.FirstOrDefault(r => r.Id == id);
            _context.Records.Remove(recordForDeletion);
            _context.SaveChanges();
            return Ok();
        }

        [HttpPut("{id}")]
        public ActionResult<Record> UpdateRecord(int id, [FromBody] Record record)
        {
            var recordForUpdate = _context.Records.FirstOrDefault(r => r.Id == id);
            
            recordForUpdate.Date = record.Date; 
            recordForUpdate.Name = record.Name; 
            recordForUpdate.Value = record.Value;
            recordForUpdate.Category = record.Category; 
            recordForUpdate.Type = record.Date; 

            _context.SaveChanges();

            return Ok();
        }
    }
}

DataContext

public class DataContext : DbContext
    {
        public DataContext(DbContextOptions<DataContext> options) : base(options) { }

        public DbSet<Record> Records { get; set; }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            //seed database with dummy data
            modelBuilder.Entity<Record>().HasData(
                new Record()
                {
                    Id = 1,
                    Date = "2020-09-30T00:00:01",
                    Name = "Coles",
                    Value = 20,
                    Category = "Groceries",
                    Type = "Expense"                   
                },

                new Record()
                {
                    Id = 2,
                    Date = "2020-10-01T00:00:01",
                    Name = "Traslink",
                    Value = 30,
                    Category = "Transportation",
                    Type = "Expense"                   
                },

                new Record()
                {
                    Id = 3,
                    Date = "2020-10-02T00:00:01",
                    Name = "Cafe 63",
                    Value = 22,
                    Category = "Eating Out",
                    Type = "Expense"                   
                }
            );
        }
    }
}

Startup:

  public class Startup
    {
        private readonly IConfiguration _config;
        public Startup(IConfiguration config)
        {
            _config = config;
        }

        // This method gets called by the runtime. Use this method to add services to the container.
        public void ConfigureServices(IServiceCollection services)
        {
            services.AddControllers();
            services.AddDbContext<DataContext>(x =>
                x.UseSqlite(_config.GetConnectionString("DefaultConnection")));
```
\$\endgroup\$
2
  • 4
    \$\begingroup\$ Keep your controllers lean: infoworld.com/article/3404472/… \$\endgroup\$
    – BCdotWEB
    Oct 22 '20 at 7:40
  • 1
    \$\begingroup\$ One minor thing I'd like to point out is the use of FirstOrDefault. If those are unique IDs (aka Primary Keys) in the data set, use SingleOrDefault to help enforce the expectation. \$\endgroup\$ Oct 22 '20 at 17:21
4
\$\begingroup\$

This implementation is quite far from a production-ready CRUD API. Just to name a few missing things:

  • Presentation and Persistence layer separation
    • Controller should deal with HTTP related stuff, not with databases
  • Proper status code handling
    • (see the next section for more details)
  • Model validation
  • Error handling
    • for example: what if database is not responding?
  • Pagination
  • Taking advantage of async I/O operations
    • It can boost scalability
  • API versioning
  • Documentation of the API
    • For example via OpenAPI
  • Authentication and Authorization
  • Audit for modifications
  • etc.

Better utilization of status codes

  • POST
    • If the Id is coming from the user and it should be unique (in order to able to do lookups) then what if the provided Id is already stored in the database? 400 / 409
    • BTW Id should not come from the user. That's why it is a good practice to separate database models and API models
    • In case of success the 201 is the suggested status code. The persisted resource could be sent back to the user inside the response body or the Location header should point to the new resource
  • GET
    • What if there is no record with the provided Id? 404
  • DELETE
    • Either 204 or 404 is used normally to indicate that a resource is gone
    • Your current implementation fails if you try to delete a non-existing record
    • Depending on your requirements you may or may not differentiate the deletion of existing and non-existing resources

Here you can find a poster, which can help you to decide when to use which http status code.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you for your answer!It provides a very good checklist to improve my app. My idea was to create an entry level api that does what it's supposed to do (the crud operations) so that the users of the tutorial have a sense of achievement quite quickly. I realised learning from tutorials that it took me weeks or even months (if I wanted to actually understand what was going on) to get something ready for production out of a tutorial. So I made something as simple as possible for someone who's starting. I'll make it clear in my tutorial that all those next steps are necessary in the industry :) \$\endgroup\$ Oct 22 '20 at 21:23

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.