4
\$\begingroup\$

I am working on architecture where I can have any client + server API. I need to call API as clean as possible and get, post, put any object I want to.

For me, it is important to have clean secure code with good practice. This is how I implemented it and I would like you to help me to improve this code.

Some of questions are:

  1. Should I return HttpResponseMessage from API (and why). Or is it ok to return list or object that I need?
  2. Should I return anything from 'post' and 'put' or leave it just void?
  3. Are HttpClientFactory and JsonContentFactory ok names for this classes? Is this factory at all?
  4. Should I return some HttpStatusCode Enumeration from API in some case? I looks to me that I get good status code without setting anything in API. But is some cased on interner I saw that they set status code in response.
  5. What is best way to handle exceptions in API?

Call API from client:

 public class WorkItemService : IWorkItemService
    {
        public async Task<IEnumerable<WorkItemDto>> GetAllWorkItems()
        {
            IEnumerable<WorkItemDto> result = null;
            using (var client = HttpClientFactory.GetClient())
            {
                var response = await client.GetAsync("api/WorkItem");
                if (response.IsSuccessStatusCode)
                {
                    string content = await response.Content.ReadAsStringAsync();
                    result = JsonConvert.DeserializeObject<IEnumerable<WorkItemDto>>(content);
                }
                if (!response.IsSuccessStatusCode)
                {
                    throw new Exception((int)response.StatusCode + "-" + response.StatusCode.ToString());
                }
            }
            return result;
        }

        public async Task AddWorkItem(WorkItemDto item)
        {
            using (var client = HttpClientFactory.GetClient())
            {
                var response = await client.PostAsync("api/WorkItem", JsonContentFactory.CreateJsonContent(item));
                if (!response.IsSuccessStatusCode)
                {
                    throw new Exception((int)response.StatusCode + "-" + response.StatusCode.ToString());
                }
            }
        }

        public async Task UpdateWorkItem(WorkItemDto item)
        {
            using (var client = HttpClientFactory.GetClient())
            {
                var response = await client.PutAsync("api/WorkItem", JsonContentFactory.CreateJsonContent(item));
                if (!response.IsSuccessStatusCode)
                {
                    throw new Exception((int)response.StatusCode + "-" + response.StatusCode.ToString());
                }
            }
        }
    }

HttpClient helper:

  public static class HttpClientFactory
    {
        public static HttpClient GetClient()
        {
            HttpClient client = new HttpClient();
            client.BaseAddress = new Uri("http://localhost:1431");

            client.DefaultRequestHeaders.Accept.Clear();
            client.DefaultRequestHeaders.Accept.Add(
                new MediaTypeWithQualityHeaderValue("application/json"));

            return client;
        }
    }

StringContent helper:

 public static class JsonContentFactory
    {
        public static StringContent CreateJsonContent(object obj)
        {
            var json = JsonConvert.SerializeObject(obj);
            var content = new StringContent(json, UnicodeEncoding.UTF8, "application/json");
            return content;
        }
    }

API:

  public class WorkItemController : ApiController
    {
        private IWorkItemService workItemService;
        private MappingEngine mapperToEntity;
        private MappingEngine mapperToDomain;

        public WorkItemController(IWorkItemService workItemService, IMapperConfiguration mapperConfiguration)
        {
            this.workItemService = workItemService;
            this.mapperToEntity = mapperConfiguration.GetMapper(MapperType.DTOToEntity);
            this.mapperToDomain = mapperConfiguration.GetMapper(MapperType.EntityToDTO);
        }

        [HttpGet]
        public async Task<IEnumerable<WorkItemDto>> Get()
        {
            var allWorkItems = await workItemService.GetAllWorkItems();
            var allWorkItemsDTO = allWorkItems.Select(item => mapperToDomain.Map<WorkItem, WorkItemDto>(item)).ToList();
            return allWorkItemsDTO;
        }

        [HttpPost]
        public async Task Post([FromBody]WorkItemDto item)
        {
            var itemEntity = mapperToEntity.Map<WorkItemDto, WorkItem>(item);
            await workItemService.Add(itemEntity);
        }

        [HttpPut]
        public async Task Put([FromBody]WorkItemDto workItem)
        {
            var itemEntity = mapperToEntity.Map<WorkItemDto, WorkItem>(workItem);
            await workItemService.Update(itemEntity);
        }
    }
\$\endgroup\$
8
\$\begingroup\$

Design

if (response.IsSuccessStatusCode)
            {
                string content = await response.Content.ReadAsStringAsync();
                result = JsonConvert.DeserializeObject<IEnumerable<WorkItemDto>>(content);
            }
            if (!response.IsSuccessStatusCode)
            {
                throw new Exception((int)response.StatusCode + "-" + response.StatusCode.ToString());
            }

These two if statements can be merged together using an else.

if (response.IsSuccessStatusCode)
            {
                string content = await response.Content.ReadAsStringAsync();
                result = JsonConvert.DeserializeObject<IEnumerable<WorkItemDto>>(content);
            }
else
            {
                throw new Exception((int)response.StatusCode + "-" + response.StatusCode.ToString());
            }

"api/WorkItem" is a magic string. Refactor this to a constant or instance variable with a good name.

By keeping "http://localhost:1431" as a hard-coded string you prevent yourself from ever being able to host the service separately. Consider putting it in a config file somewhere and reading it at runtime.

I cannot see any code branch that allows this to return null, so why are you initializing the result to null in the first place?

public async Task<IEnumerable<WorkItemDto>> GetAllWorkItems()
    {
        IEnumerable<WorkItemDto> result = null;
        using (var client = HttpClientFactory.GetClient())
        {
            var response = await client.GetAsync("api/WorkItem");
            if (response.IsSuccessStatusCode)
            {
                string content = await response.Content.ReadAsStringAsync();
                result = JsonConvert.DeserializeObject<IEnumerable<WorkItemDto>>(content);
            }
            if (!response.IsSuccessStatusCode)
            {
                throw new Exception((int)response.StatusCode + "-" + response.StatusCode.ToString());
            }
        }
        return result;
    }

Instead, simply return the value when you have it.

public async Task<IEnumerable<WorkItemDto>> GetAllWorkItems()
    {
        using (var client = HttpClientFactory.GetClient())
        {
            var response = await client.GetAsync("api/WorkItem");
            if (response.IsSuccessStatusCode)
            {
                string content = await response.Content.ReadAsStringAsync();
                return JsonConvert.DeserializeObject<IEnumerable<WorkItemDto>>(content);
            }
            else
            {
                throw new Exception((int)response.StatusCode + "-" + response.StatusCode.ToString());
            }
        }
    }

Exceptions

if (!response.IsSuccessStatusCode)
            {
                throw new Exception((int)response.StatusCode + "-" + response.StatusCode.ToString());
            }

Don't throw generic Exception types, use a more explicit type that explains what went wrong, don't just leave it to the exception message to explain because that makes it so much harder to catch.

Var

Your usage of implicit typing is great for most of the code, but you've got this line.

HttpClient client = new HttpClient();

That should be

var client = new HttpClient();

With style guides, consistency is more important than whether you do or do not follow a particular rule.

Naming

Your parameter naming is inconsistent.

public async Task Post([FromBody]WorkItemDto item)

public async Task Put([FromBody]WorkItemDto workItem)
\$\endgroup\$
  • \$\begingroup\$ Thank for help. This are all good sugestions. I updated my post with some questions that I still have. \$\endgroup\$ – Raskolnikov Nov 26 '15 at 11:18
6
\$\begingroup\$

@Nick already has covered some important stuff, so I'd add my two cents.

Nick pointed out that :

if (response.IsSuccessStatusCode)
{
    string content = await response.Content.ReadAsStringAsync();
    result = JsonConvert.DeserializeObject<IEnumerable<WorkItemDto>>(content);
}
if (!response.IsSuccessStatusCode)
{
    throw new Exception((int)response.StatusCode + "-" + response.StatusCode.ToString());
}

is weird. You can switch it for a else, but you can also remove the else entirely if you reverse your condition. Meaning :

if (!response.IsSuccessStatusCode)
{
    throw new Exception((int)response.StatusCode + "-" + response.StatusCode.ToString());
}

string content = await response.Content.ReadAsStringAsync();
result = JsonConvert.DeserializeObject<IEnumerable<WorkItemDto>>(content);

This way you reduce the nesting and you improve readability.

Also, instead of throwing Exception, consider throwing a custom exception. You always need the same format :

(int)response.StatusCode + "-" + response.StatusCode.ToString()

Well, why don't you create an exception that works with this? Something like ApiCallFailedException (You might have a more appropriate/domain-specific exception name!) :

class ApiCallFailedException : ApplicationException
{
    public ApiCallFailedException(??? statusCode)
    {
        Message = $"{(int)StatusCode} - {StatusCode}";
    }
}

Now, see what I did there? I don't know the type for response, because you used var at a moment where we can't figure the type by reading.

@Heslasher pointed in comments that response wouldn't be a good paramter for the exception and it's true. Doing this you introduce a bond between two classes that shouldn't know each other. So instead I passed the StatusCode, but I don't know what's the type!

var should be used only when you can tell the type by reading the code. Ex :

var someCode = "550032423"; //good
var foobar = GetFooBar(); //bad, what is foobar?

We can extract constants out of your code. The best example possible is "api/WorkItem". What if it happened to change? You'd need to change it at three places. You might forget one and boom, you introduced a bug.

How do we fix this? With a constant!

const string workItemApi = "api/WorkItem";

You can then reuse this constant everywhere in your code without fear of breaking something if it happened to change.

This : "http://localhost:1431" should be extracted from a configuration file (Like the app.config). What if you had to change the port or the host of your application? You'd need to recompile your application. This isn't good.

It might be a mistake when you copied your code here, but your indentation is off :

public class WorkItemController : ApiController
  {

should be :

public class WorkItemController : ApiController
{

Finally, you could set your private member to readonly since they don't seem to be modifier inside your controller.

    private readonly IWorkItemService workItemService;
    private readonly MappingEngine mapperToEntity;
    private readonly MappingEngine mapperToDomain;

I don't know what mapping engine you use, but do you know about Automapper? It's really awesome.

\$\endgroup\$
  • \$\begingroup\$ Nice catch on reversing the condition. \$\endgroup\$ – Nick Udell Nov 26 '15 at 15:34
  • 2
    \$\begingroup\$ Using a custom exception is the way to go, but the response shouldn't be passed to the constructor because it is tightly coupling the exception to the response where the calling client maybe isn't aware of the response. \$\endgroup\$ – Heslacher Nov 26 '15 at 15:38

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.