3
\$\begingroup\$

I have been coding in java for more than 3 years. Recently we had to develop REST Web Services for our customers in ASP.NET Core. I am a newbie to both C# and ASP.NET Core stack.

The expectation behind asking this question here is to get suggestions and best practices which needs to be followed to make my API readable as well as best in design.

Usecase:

The API is a service to create,update,delete and access User Settings for all the users having access to various applications. This API is deployed as a microservice.

Controller:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Mvc;
using Zeiss.IMT.MCCNeo.UserSettings.Data.Settings.Repositiories;
using Zeiss.IMT.MCCNeo.UserSettings.Entities;

namespace Zeiss.IMT.MCCNeo.UserSettings.API.Controllers
{
    [ApiVersion("1.0")]
    [Route("api/v{version:apiVersion}/[controller]")]
    public class UserSettingController : Controller
    {
        private readonly UserSettingRepository _userSettingRepository;

        public UserSettingController()
        {
            _userSettingRepository = new UserSettingRepository();
        }

        [HttpGet]
        public IActionResult Get()
        {
            try
            {
                var result = _userSettingRepository.Get();

                return Ok(result);
            }
            catch (Exception ex)
            {
                //ToDO:Exception handling.....
                return BadRequest(ex);
            }


        }

        [HttpGet("{userid}/{settingKey}")]
        public IActionResult Get(string userid, string settingKey)
        {
            try
            {
                var result = _userSettingRepository.SettingsByKeyUserID(settingKey, userid);

                return Ok(result);
            }
            catch (Exception ex)
            {
                return BadRequest(ex);
            }


        }

        [HttpGet("{settingkey}")]
        public IActionResult Get(string settingkey)
        {
            try
            {
                var result = _userSettingRepository.SettingsByKey(settingkey);
                return Ok(result);
            }
            catch (Exception ex)
            {
                return BadRequest(ex);
            }
        }

        [HttpPost]
        public IActionResult Post([FromBody] UserSettingEntity entity)
        {
            var result = _userSettingRepository.Save(entity);
            if (result == true)
            {
                return Ok();
            }
            else
            {
                return BadRequest();
            }
        }

    }
}

Model:

using System;
using System.Collections.Generic;
using System.Text;

namespace Zeiss.IMT.MCCNeo.UserSettings.Entities
{
    public class UserSettingEntity
    {
        public string UserID { get; set; }
        public string ApplicationName { get; set; }
        public string SettingKey { get; set; }
        public string SettingValue { get; set; }
        public bool IsActive { get; set; }
        public DateTime UpdatedOn { get; set; }
        public string SettingLabel { get; set; }

    }
}

Repository Interfaces and Classes:

using System;
using System.Collections.Generic;
using System.Text;

using System.Linq;
using Zeiss.IMT.MCCNeo.UserSettings.Data.Interfaces;
using Zeiss.IMT.MCCNeo.UserSettings.Entities;

namespace Zeiss.IMT.MCCNeo.UserSettings.Data.Settings.Repositiories
{
    public class UserSettingRepository : IUserSettingRepository
    {
        private IList<UserSettingEntity> Settings = new List<UserSettingEntity>();

        public UserSettingRepository() {
            Settings.Add(new UserSettingEntity() { UserID = "alpha", ApplicationName = "protocol archive", SettingKey = "RefreshTime", SettingValue = "20", UpdatedOn = new DateTime().Date, SettingLabel = "RefreshInterval", IsActive = true });
            Settings.Add(new UserSettingEntity() { UserID = "bravo", ApplicationName = "view", SettingKey = "RefreshTime", SettingValue = "20", UpdatedOn = new DateTime().Date, SettingLabel = "RefreshInterval", IsActive = true });
            Settings.Add(new UserSettingEntity() { UserID = "charlie", ApplicationName = "upload", SettingKey = "RefreshTime", SettingValue = "20", UpdatedOn = new DateTime().Date, SettingLabel = "RefreshInterval", IsActive = true });
            Settings.Add(new UserSettingEntity() { UserID = "foxtrot", ApplicationName = "protocol archive", SettingKey = "RefreshTime", SettingValue = "20", UpdatedOn = new DateTime().Date, SettingLabel = "RefreshInterval", IsActive = true });
            Settings.Add(new UserSettingEntity() { UserID = "lima", ApplicationName = "view", SettingKey = "RefreshTime", SettingValue = "20", UpdatedOn = new DateTime().Date, SettingLabel = "RefreshInterval", IsActive = true });
            Settings.Add(new UserSettingEntity() { UserID = "Julie", ApplicationName = "upload", SettingKey = "RefreshTime", SettingValue = "20", UpdatedOn = new DateTime().Date, SettingLabel = "RefreshInterval", IsActive = true });
            Settings.Add(new UserSettingEntity() { UserID = "tango", ApplicationName = "protocol archive", SettingKey = "RefreshTime", SettingValue = "20", UpdatedOn = new DateTime().Date, SettingLabel = "RefreshInterval", IsActive = true });
            Settings.Add(new UserSettingEntity() { UserID = "victor", ApplicationName = "protocol archive", SettingKey = "RefreshTime", SettingValue = "20", UpdatedOn = new DateTime().Date, SettingLabel = "RefreshInterval", IsActive = true });
            Settings.Add(new UserSettingEntity() { UserID = "yankee", ApplicationName = "protocol archive", SettingKey = "RefreshTime", SettingValue = "20", UpdatedOn = new DateTime().Date, SettingLabel = "RefreshInterval", IsActive = true });
            Settings.Add(new UserSettingEntity() { UserID = "zulu", ApplicationName = "protocol archive", SettingKey = "RefreshTime", SettingValue = "20", UpdatedOn = new DateTime().Date, SettingLabel = "RefreshInterval", IsActive = true });
        }

        public IEnumerable<UserSettingEntity> Get() {
            return Settings.ToList<UserSettingEntity>();
        }  

        public bool Save(UserSettingEntity entity)
        {
            var flag = false;
            if (entity != null)
            {
                var existing = Settings.Where(se => se.UserID == entity.UserID && se.ApplicationName == entity.ApplicationName && se.SettingKey == entity.SettingKey).ToList();
                if (existing != null)
                {
                    existing.ForEach(f => f.SettingValue = entity.SettingValue);
                    flag = true;
                }
                else
                {
                    Settings.Add(entity);
                    flag = true;
                }
                return flag;
            }
            else {
                throw new ArgumentNullException();
            }


        }



        public IEnumerable<UserSettingEntity> SettingsByKey(string settingKey)
        {
            return Settings.Where(se => se.SettingKey == settingKey).ToList();
        }

        IEnumerable<UserSettingEntity> IUserSettingRepository.Settings(string userID)
        {
            return Settings.Where(se => se.UserID == userID).ToList();
        }

        public IEnumerable<UserSettingEntity> SettingsByKeyUserID(string settingKey,string userid )
        {
            return Settings.Where(se => se.UserID == userid && se.SettingKey == settingKey).ToList();
        }
    }
}



using System;
using System.Collections.Generic;
using System.Text;
using Zeiss.IMT.MCCNeo.UserSettings.Entities;

namespace Zeiss.IMT.MCCNeo.UserSettings.Data.Interfaces
{
    public interface IUserSettingRepository
    {
        void Save(UserSettingEntity entity);
        IEnumerable<UserSettingEntity> Settings(string userID);

        IEnumerable<UserSettingEntity> SettingsByKey(string settingKey);
    }
}

I have been referring to the blog below

http://www.vinaysahni.com/best-practices-for-a-pragmatic-restful-api

\$\endgroup\$
3
\$\begingroup\$

Exception handling

There is no need to do a try/catch if you aren't going to handle them differently, let the global exception handler handle them and do all the logging.

Save Method

The "existing" variable will never be null, If there is no match you will get an empty list.

The name of the method doesn't really tell what it's doing, it will do an update if it's already there and it will add a new one if it doesn't exist. consider renaming.

Guard clauses not if-else

if (entity == null)
    throw new ArgumentNullException();
..
..

Validation

You should validate the input of your controller actions. model validation

Separating models and entities

models and entities

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

It's best to have your controllers run async by declaring them like this.

    [HttpGet]
    public Task<IActionResult> Get()

The server still has to wait for the entire request to be ready before returning it to the client, but declaring them as promises frees your server up to handle more requests "simultaneously". This way, the server can begin processing the next request while it waits on the first one to finish processing.

Doing it the way you did it means that each request will block until it's finished. If you get many requests, it can become a problem.


I also noticed that you're not logging exceptions. You'll want to do that before going to prod. Your braces are also inconsistent. Pick a style and stick with it (or better, run an auto formatter on save).

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