Clean call api from client, with good practice

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:

• Should I return HttpResponseMessage from API (and why). Or is it ok to return list or object that I need?

• Should I return anything from 'post' and 'put' or leave it just void?

• Are HttpClientFactory and JsonContentFactory ok names for this classes? Is this factory at all?

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

• What is best way to handle exceptions in API?

Call API from client:

```
public class WorkItemService : IWorkItemService
{
public async Task> GetAllWorkItems()
{
IEnumerable 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>(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.IsSuccessStatus

Design

if (response.IsSuccessStatusCode)
            {
                string content = await response.Content.ReadAsStringAsync();
                result = JsonConvert.DeserializeObject>(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>(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> GetAllWorkItems()
    {
        IEnumerable 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>(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> 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>(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)

StackExchange Code Review Q#111882, answer score: 9