Using coalescing and conditional operators to check API arguments
up vote
1
down vote
favorite
I have a web application that allows users to create customs items and later change those items. For eg. a change, the application takes the parameters and performs some basic sanity checks.
If the checks pass, a REST service is called and the result of the response is converted to an ActionResult that can then be further handled by the application.
Is the checks fail, I currently create my own HttpResponseMessage that can then also be converted to an ActionResult:
public class HomeController : Controller
{
private readonly IRoleManagerServiceAccess _access;
private const string TEXT_PLAIN = "text/plain";
public HomeController(IRoleManagerServiceAccess access)
{
_access = access;
}
[HttpPut("[action]/{groupImportId?}")]
public async Task<ActionResult<DisplayGroupDto>> ChangeGroupDescription(string groupImportId, [FromBody] string groupDescription)
{
var serviceResponse = await _access.ChangeGroupDescriptionAsync(groupImportId, groupDescription);
if(serviceResponse.IsSuccessStatusCode)
{
return await serviceResponse.Content.ReadAsAsync<DisplayGroupDto>();
}
return new ContentResult
{
StatusCode = (int?)serviceResponse.StatusCode,
Content = await serviceResponse.Content.ReadAsStringAsync(),
ContentType = TEXT_PLAIN
};
}
}
public sealed class RoleManagerServiceAccess : IRoleManagerServiceAccess
{
private readonly HttpClient _httpClient;
public RoleManagerServiceAccess(HttpClient httpClient)
{
_httpClient = httpClient;
public async Task<HttpResponseMessage> ChangeGroupDescriptionAsync(string groupImportId, string groupDescription)
{
return
changeGroupDescriptionParamsAreInvalid(groupImportId, groupDescription) ??
await changeGroupDescriptionParamsAreValidAsync(groupImportId, groupDescription);
}
private async Task<HttpResponseMessage> changeGroupDescriptionParamsAreValidAsync(string groupImportId, string groupDescription)
{
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Description, groupDescription.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
private static HttpResponseMessage changeGroupDescriptionParamsAreInvalid(string groupImportId, string groupDescription)
{
return groupImportIdIsInvalid(groupImportId)
?? groupDescriptionIsInvalid(groupDescription);
}
private static HttpResponseMessage groupDescriptionIsInvalid(string groupDescription)
{
return groupDescription == null
? new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent("Please provide a non-null value for the group description.") }
: null;
}
private static HttpResponseMessage groupImportIdIsInvalid(string groupImportId)
{
return String.IsNullOrWhiteSpace(groupImportId)
? new HttpResponseMessage { StatusCode = HttpStatusCode.BadRequest, Content = new StringContent("Please provide a non-whitespace groupImportId.") }
: null;
}
public async Task<HttpResponseMessage> ChangeGroupImportIdAsync(string oldGroupImportId, string newGroupImportId)
{
if (String.IsNullOrWhiteSpace(oldGroupImportId))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace value for the old group importId.")
};
}
if (String.IsNullOrWhiteSpace(newGroupImportId))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace value for the new group importId.")
};
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.ImportId, newGroupImportId.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(oldGroupImportId, jsonPatch));
}
public async Task<HttpResponseMessage> CreateGroupAsync(string parentGroupImportId, CreateGroupDto createGroupDto)
{
if (createGroupDto == null)
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-null group for creation")
};
}
if (String.IsNullOrWhiteSpace(createGroupDto.Name))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace groupName.")
};
}
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(createGroupDto.ImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
if (String.IsNullOrWhiteSpace(parentGroupImportId))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace parentGroupImportId.")
};
}
return await _httpClient.PostAsJsonAsync<CreateGroupDto>($"api/group/{parentGroupImportId}", createGroupDto);
}
public async Task<HttpResponseMessage> DeleteGroupAsync(string groupImportId, bool forceDelete)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.DeleteAsync($"api/group/{groupImportId}?forceDelete={forceDelete}");
}
public async Task<HttpResponseMessage> GetGroupAsync(string groupImportId)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.GetAsync($"/api/group/{groupImportId}");
}
public async Task<HttpResponseMessage> RenameGroupAsync(string groupImportId, string groupName)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
if (String.IsNullOrWhiteSpace(groupName))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace value for the group name.")
};
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Name, groupName.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
private static HttpRequestMessage preparePatchRequestMessage(string groupImportId, JsonPatchDocument<CreateGroupDto> jsonPatch)
{
return new HttpRequestMessage
{
Method = HttpMethod.Patch,
RequestUri = new Uri($"api/group/{groupImportId}", UriKind.Relative),
Content = new ObjectContent<JsonPatchDocument<CreateGroupDto>>(jsonPatch, new JsonMediaTypeFormatter())
};
}
public async Task<HttpResponseMessage> SetRulePartsAsync(string groupImportId, List<CreateRulePartDto> ruleParts)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
if (ruleParts == null)
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a list of rule parts.")
};
}
return await _httpClient.PutAsJsonAsync<List<CreateRulePartDto>>($"api/group/{groupImportId}/ruleParts", ruleParts);
}
public async Task<HttpResponseMessage> GetGroupMembersAsync(string groupImportId)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.GetAsync($"api/group/{groupImportId}/members");
}
public async Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members)
{
if (members == null)
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a valid members list")
};
}
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.PutAsJsonAsync<List<string>>($"api/group/{groupImportId}/members", members);
}
}
As you can see, in the RoleManagerServiceAccess code, I make extensive use of the conditional and the null-conditional operators to perform my sanity checks. This allows me to return a HttpResponseMessage both in the check-failing and the check-passing cases. I don't have to throw exceptions that I then would have to handle in the HomeController in order to translate them to an ActionResult but can directly use the response from the RoleManagerServiceAccess, be it a "real" response from the REST service or a "constructed" response because the checks are failing.
Of course, the ChangeGroupDescription is not the only method there is, the sanity checks are mostly the same across all methods.
Now the question is: Is this a good idea? Is it clear what is going on here or does it only help in obfuscating the code's intent?
Thanks for any input!
EDIT
Trying to provide some more context.
The interface looks like this:
public interface IRoleManagerServiceAccess
{
Task<HttpResponseMessage> GetGroupAsync(string groupImportId);
Task<HttpResponseMessage> CreateGroupAsync(string parentGroupImportId, Dto.CreateGroupDto createGroupDto);
Task<HttpResponseMessage> DeleteGroupAsync(string groupImportId, bool forceDelete);
Task<HttpResponseMessage> SetRulePartsAsync(string groupImportId, List<CreateRulePartDto> ruleParts);
Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members);
Task<HttpResponseMessage> RenameGroupAsync(string groupImportId, string groupName);
Task<HttpResponseMessage> ChangeGroupDescriptionAsync(string groupImportId, string groupDescription);
Task<HttpResponseMessage> ChangeGroupImportIdAsync(string oldGroupImportId, string newGroupImportId);
Task<HttpResponseMessage> GetGroupMembersAsync(string groupImportId);
}
Each of these methods gets passed the same parameter groupImportId and possibly, depending on functionality, an additional parameter. In each of these methods, I check if groupImportId is whitespace, if that is the case, I don't need to call the REST service. If an additional parameter is passed, as in the case of ChangeGroupDescription, it is checked for constraints also and if it fails, the REST service is not called.
If all checks for the parameters pass, the service is called with the parameters and the response is passed back to the caller. The service response's status code can indicate a success, in which case I want the HomeController to return the deserialized object of the response, or it can indicate a failure, in which case I want the HomeController to return a HttpResponseMessage with the same StatusCode and Content as the service response's.
In the case of a failing sanity check, I want the HomeController to also return a ContentResult with code 400 and some Content. Previously, I had thrown an ArgumentNullException in the RoleManagerServiceAccess methods; in each method of HomeController, I had to wrap the call to _access.xyz() in a try-catch-block, where I would examine the exception's message and build the ContentResult from it. That lead to nine times the same try-catch-block in HomeController and nine times the same throw new ArgumentNullException() block in RoleManagerServiceAccess. I thought there might be a better way.
Another edit:
I've reverted my code to a version without coalesing and conditional operators. I think it is clearer now what is happening, although I'm still not happy with the overall structure.
public sealed class RoleManagerServiceAccess : IRoleManagerServiceAccess
{
private readonly HttpClient _httpClient;
private const string GROUPIMPORTID_BADREQUEST_MESSAGE = "Please provide a non-whitespace groupImportId.";
public RoleManagerServiceAccess(HttpClient httpClient)
{
_httpClient = httpClient;
}
public async Task<HttpResponseMessage> ChangeGroupDescriptionAsync(string groupImportId, string groupDescription)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (groupDescription == null)
{
return createBadRequestResponse("Please provide a non-null value for the group description.");
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Description, groupDescription.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
public async Task<HttpResponseMessage> ChangeGroupImportIdAsync(string oldGroupImportId, string newGroupImportId)
{
if (String.IsNullOrWhiteSpace(oldGroupImportId))
{
return createBadRequestResponse("Please provide a non-whitespace value for the old group importId.");
}
if (String.IsNullOrWhiteSpace(newGroupImportId))
{
return createBadRequestResponse("Please provide a non-whitespace value for the new group importId.");
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.ImportId, newGroupImportId.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(oldGroupImportId, jsonPatch));
}
public async Task<HttpResponseMessage> CreateGroupAsync(string parentGroupImportId, CreateGroupDto createGroupDto)
{
if (createGroupDto == null)
{
return createBadRequestResponse("Please provide a non-null group for creation");
}
if (String.IsNullOrWhiteSpace(createGroupDto.Name))
{
return createBadRequestResponse("Please provide a non-whitespace groupName.");
}
if (String.IsNullOrWhiteSpace(createGroupDto.ImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (String.IsNullOrWhiteSpace(parentGroupImportId))
{
return createBadRequestResponse("Please provide a non-whitespace parentGroupImportId.");
}
return await _httpClient.PostAsJsonAsync<CreateGroupDto>($"api/group/{parentGroupImportId}", createGroupDto);
}
public async Task<HttpResponseMessage> DeleteGroupAsync(string groupImportId, bool forceDelete)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.DeleteAsync($"api/group/{groupImportId}?forceDelete={forceDelete}");
}
public async Task<HttpResponseMessage> GetGroupAsync(string groupImportId)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.GetAsync($"/api/group/{groupImportId}");
}
public async Task<HttpResponseMessage> RenameGroupAsync(string groupImportId, string groupName)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (String.IsNullOrWhiteSpace(groupName))
{
return createBadRequestResponse("Please provide a non-whitespace value for the group name.");
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Name, groupName.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
private static HttpRequestMessage preparePatchRequestMessage(string groupImportId, JsonPatchDocument<CreateGroupDto> jsonPatch)
{
return new HttpRequestMessage
{
Method = HttpMethod.Patch,
RequestUri = new Uri($"api/group/{groupImportId}", UriKind.Relative),
Content = new ObjectContent<JsonPatchDocument<CreateGroupDto>>(jsonPatch, new JsonMediaTypeFormatter())
};
}
public async Task<HttpResponseMessage> SetRulePartsAsync(string groupImportId, List<CreateRulePartDto> ruleParts)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (ruleParts == null)
{
return createBadRequestResponse("Please provide a list of rule parts.");
}
return await _httpClient.PutAsJsonAsync<List<CreateRulePartDto>>($"api/group/{groupImportId}/ruleParts", ruleParts);
}
public async Task<HttpResponseMessage> GetGroupMembersAsync(string groupImportId)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.GetAsync($"api/group/{groupImportId}/members");
}
public async Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members)
{
if (members == null)
{
return createBadRequestResponse("Please provide a valid members list");
}
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.PutAsJsonAsync<List<string>>($"api/group/{groupImportId}/members", members);
}
private static HttpResponseMessage createBadRequestResponse(string message) =>
new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent(message) };
}
c# rest null asp.net-web-api
asked Jun 14 at 9:46
Thaoden
1266
|
show 2 more comments
up vote
1
down vote
favorite
I have a web application that allows users to create customs items and later change those items. For eg. a change, the application takes the parameters and performs some basic sanity checks.
If the checks pass, a REST service is called and the result of the response is converted to an ActionResult that can then be further handled by the application.
Is the checks fail, I currently create my own HttpResponseMessage that can then also be converted to an ActionResult:
public class HomeController : Controller
{
private readonly IRoleManagerServiceAccess _access;
private const string TEXT_PLAIN = "text/plain";
public HomeController(IRoleManagerServiceAccess access)
{
_access = access;
}
[HttpPut("[action]/{groupImportId?}")]
public async Task<ActionResult<DisplayGroupDto>> ChangeGroupDescription(string groupImportId, [FromBody] string groupDescription)
{
var serviceResponse = await _access.ChangeGroupDescriptionAsync(groupImportId, groupDescription);
if(serviceResponse.IsSuccessStatusCode)
{
return await serviceResponse.Content.ReadAsAsync<DisplayGroupDto>();
}
return new ContentResult
{
StatusCode = (int?)serviceResponse.StatusCode,
Content = await serviceResponse.Content.ReadAsStringAsync(),
ContentType = TEXT_PLAIN
};
}
}
public sealed class RoleManagerServiceAccess : IRoleManagerServiceAccess
{
private readonly HttpClient _httpClient;
public RoleManagerServiceAccess(HttpClient httpClient)
{
_httpClient = httpClient;
public async Task<HttpResponseMessage> ChangeGroupDescriptionAsync(string groupImportId, string groupDescription)
{
return
changeGroupDescriptionParamsAreInvalid(groupImportId, groupDescription) ??
await changeGroupDescriptionParamsAreValidAsync(groupImportId, groupDescription);
}
private async Task<HttpResponseMessage> changeGroupDescriptionParamsAreValidAsync(string groupImportId, string groupDescription)
{
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Description, groupDescription.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
private static HttpResponseMessage changeGroupDescriptionParamsAreInvalid(string groupImportId, string groupDescription)
{
return groupImportIdIsInvalid(groupImportId)
?? groupDescriptionIsInvalid(groupDescription);
}
private static HttpResponseMessage groupDescriptionIsInvalid(string groupDescription)
{
return groupDescription == null
? new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent("Please provide a non-null value for the group description.") }
: null;
}
private static HttpResponseMessage groupImportIdIsInvalid(string groupImportId)
{
return String.IsNullOrWhiteSpace(groupImportId)
? new HttpResponseMessage { StatusCode = HttpStatusCode.BadRequest, Content = new StringContent("Please provide a non-whitespace groupImportId.") }
: null;
}
public async Task<HttpResponseMessage> ChangeGroupImportIdAsync(string oldGroupImportId, string newGroupImportId)
{
if (String.IsNullOrWhiteSpace(oldGroupImportId))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace value for the old group importId.")
};
}
if (String.IsNullOrWhiteSpace(newGroupImportId))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace value for the new group importId.")
};
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.ImportId, newGroupImportId.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(oldGroupImportId, jsonPatch));
}
public async Task<HttpResponseMessage> CreateGroupAsync(string parentGroupImportId, CreateGroupDto createGroupDto)
{
if (createGroupDto == null)
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-null group for creation")
};
}
if (String.IsNullOrWhiteSpace(createGroupDto.Name))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace groupName.")
};
}
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(createGroupDto.ImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
if (String.IsNullOrWhiteSpace(parentGroupImportId))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace parentGroupImportId.")
};
}
return await _httpClient.PostAsJsonAsync<CreateGroupDto>($"api/group/{parentGroupImportId}", createGroupDto);
}
public async Task<HttpResponseMessage> DeleteGroupAsync(string groupImportId, bool forceDelete)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.DeleteAsync($"api/group/{groupImportId}?forceDelete={forceDelete}");
}
public async Task<HttpResponseMessage> GetGroupAsync(string groupImportId)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.GetAsync($"/api/group/{groupImportId}");
}
public async Task<HttpResponseMessage> RenameGroupAsync(string groupImportId, string groupName)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
if (String.IsNullOrWhiteSpace(groupName))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace value for the group name.")
};
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Name, groupName.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
private static HttpRequestMessage preparePatchRequestMessage(string groupImportId, JsonPatchDocument<CreateGroupDto> jsonPatch)
{
return new HttpRequestMessage
{
Method = HttpMethod.Patch,
RequestUri = new Uri($"api/group/{groupImportId}", UriKind.Relative),
Content = new ObjectContent<JsonPatchDocument<CreateGroupDto>>(jsonPatch, new JsonMediaTypeFormatter())
};
}
public async Task<HttpResponseMessage> SetRulePartsAsync(string groupImportId, List<CreateRulePartDto> ruleParts)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
if (ruleParts == null)
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a list of rule parts.")
};
}
return await _httpClient.PutAsJsonAsync<List<CreateRulePartDto>>($"api/group/{groupImportId}/ruleParts", ruleParts);
}
public async Task<HttpResponseMessage> GetGroupMembersAsync(string groupImportId)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.GetAsync($"api/group/{groupImportId}/members");
}
public async Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members)
{
if (members == null)
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a valid members list")
};
}
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.PutAsJsonAsync<List<string>>($"api/group/{groupImportId}/members", members);
}
}
As you can see, in the RoleManagerServiceAccess code, I make extensive use of the conditional and the null-conditional operators to perform my sanity checks. This allows me to return a HttpResponseMessage both in the check-failing and the check-passing cases. I don't have to throw exceptions that I then would have to handle in the HomeController in order to translate them to an ActionResult but can directly use the response from the RoleManagerServiceAccess, be it a "real" response from the REST service or a "constructed" response because the checks are failing.
Of course, the ChangeGroupDescription is not the only method there is, the sanity checks are mostly the same across all methods.
Now the question is: Is this a good idea? Is it clear what is going on here or does it only help in obfuscating the code's intent?
Thanks for any input!
EDIT
Trying to provide some more context.
The interface looks like this:
public interface IRoleManagerServiceAccess
{
Task<HttpResponseMessage> GetGroupAsync(string groupImportId);
Task<HttpResponseMessage> CreateGroupAsync(string parentGroupImportId, Dto.CreateGroupDto createGroupDto);
Task<HttpResponseMessage> DeleteGroupAsync(string groupImportId, bool forceDelete);
Task<HttpResponseMessage> SetRulePartsAsync(string groupImportId, List<CreateRulePartDto> ruleParts);
Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members);
Task<HttpResponseMessage> RenameGroupAsync(string groupImportId, string groupName);
Task<HttpResponseMessage> ChangeGroupDescriptionAsync(string groupImportId, string groupDescription);
Task<HttpResponseMessage> ChangeGroupImportIdAsync(string oldGroupImportId, string newGroupImportId);
Task<HttpResponseMessage> GetGroupMembersAsync(string groupImportId);
}
Each of these methods gets passed the same parameter groupImportId and possibly, depending on functionality, an additional parameter. In each of these methods, I check if groupImportId is whitespace, if that is the case, I don't need to call the REST service. If an additional parameter is passed, as in the case of ChangeGroupDescription, it is checked for constraints also and if it fails, the REST service is not called.
If all checks for the parameters pass, the service is called with the parameters and the response is passed back to the caller. The service response's status code can indicate a success, in which case I want the HomeController to return the deserialized object of the response, or it can indicate a failure, in which case I want the HomeController to return a HttpResponseMessage with the same StatusCode and Content as the service response's.
In the case of a failing sanity check, I want the HomeController to also return a ContentResult with code 400 and some Content. Previously, I had thrown an ArgumentNullException in the RoleManagerServiceAccess methods; in each method of HomeController, I had to wrap the call to _access.xyz() in a try-catch-block, where I would examine the exception's message and build the ContentResult from it. That lead to nine times the same try-catch-block in HomeController and nine times the same throw new ArgumentNullException() block in RoleManagerServiceAccess. I thought there might be a better way.
Another edit:
I've reverted my code to a version without coalesing and conditional operators. I think it is clearer now what is happening, although I'm still not happy with the overall structure.
public sealed class RoleManagerServiceAccess : IRoleManagerServiceAccess
{
private readonly HttpClient _httpClient;
private const string GROUPIMPORTID_BADREQUEST_MESSAGE = "Please provide a non-whitespace groupImportId.";
public RoleManagerServiceAccess(HttpClient httpClient)
{
_httpClient = httpClient;
}
public async Task<HttpResponseMessage> ChangeGroupDescriptionAsync(string groupImportId, string groupDescription)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (groupDescription == null)
{
return createBadRequestResponse("Please provide a non-null value for the group description.");
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Description, groupDescription.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
public async Task<HttpResponseMessage> ChangeGroupImportIdAsync(string oldGroupImportId, string newGroupImportId)
{
if (String.IsNullOrWhiteSpace(oldGroupImportId))
{
return createBadRequestResponse("Please provide a non-whitespace value for the old group importId.");
}
if (String.IsNullOrWhiteSpace(newGroupImportId))
{
return createBadRequestResponse("Please provide a non-whitespace value for the new group importId.");
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.ImportId, newGroupImportId.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(oldGroupImportId, jsonPatch));
}
public async Task<HttpResponseMessage> CreateGroupAsync(string parentGroupImportId, CreateGroupDto createGroupDto)
{
if (createGroupDto == null)
{
return createBadRequestResponse("Please provide a non-null group for creation");
}
if (String.IsNullOrWhiteSpace(createGroupDto.Name))
{
return createBadRequestResponse("Please provide a non-whitespace groupName.");
}
if (String.IsNullOrWhiteSpace(createGroupDto.ImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (String.IsNullOrWhiteSpace(parentGroupImportId))
{
return createBadRequestResponse("Please provide a non-whitespace parentGroupImportId.");
}
return await _httpClient.PostAsJsonAsync<CreateGroupDto>($"api/group/{parentGroupImportId}", createGroupDto);
}
public async Task<HttpResponseMessage> DeleteGroupAsync(string groupImportId, bool forceDelete)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.DeleteAsync($"api/group/{groupImportId}?forceDelete={forceDelete}");
}
public async Task<HttpResponseMessage> GetGroupAsync(string groupImportId)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.GetAsync($"/api/group/{groupImportId}");
}
public async Task<HttpResponseMessage> RenameGroupAsync(string groupImportId, string groupName)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (String.IsNullOrWhiteSpace(groupName))
{
return createBadRequestResponse("Please provide a non-whitespace value for the group name.");
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Name, groupName.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
private static HttpRequestMessage preparePatchRequestMessage(string groupImportId, JsonPatchDocument<CreateGroupDto> jsonPatch)
{
return new HttpRequestMessage
{
Method = HttpMethod.Patch,
RequestUri = new Uri($"api/group/{groupImportId}", UriKind.Relative),
Content = new ObjectContent<JsonPatchDocument<CreateGroupDto>>(jsonPatch, new JsonMediaTypeFormatter())
};
}
public async Task<HttpResponseMessage> SetRulePartsAsync(string groupImportId, List<CreateRulePartDto> ruleParts)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (ruleParts == null)
{
return createBadRequestResponse("Please provide a list of rule parts.");
}
return await _httpClient.PutAsJsonAsync<List<CreateRulePartDto>>($"api/group/{groupImportId}/ruleParts", ruleParts);
}
public async Task<HttpResponseMessage> GetGroupMembersAsync(string groupImportId)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.GetAsync($"api/group/{groupImportId}/members");
}
public async Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members)
{
if (members == null)
{
return createBadRequestResponse("Please provide a valid members list");
}
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.PutAsJsonAsync<List<string>>($"api/group/{groupImportId}/members", members);
}
private static HttpResponseMessage createBadRequestResponse(string message) =>
new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent(message) };
}
c# rest null asp.net-web-api
asked Jun 14 at 9:46
Thaoden
1266
1
This is super-confusing! You callchangeGroupDescriptionParamsAreInvalidas if you already knew that parameters are invalid then you try to call the other API because your guess about invalid parameters failed. This is horrible :-| You have a single public API. Just check the parameters and handle it accordingly instead of trying to guess it in several chained methods. This is not at all clever.
– t3chb0t
Jun 14 at 11:11
@t3chb0t Thanks for your input. I'm trying to not clutter my code with the same sanity checks all over the place and not to have duplicate code between raising an exception and handling that same exception. Any hints how to get there?
– Thaoden
Jun 14 at 11:30
1
You have to provide more code if you want us to see patterns. Seeing only this I can only say what I've already said.
– t3chb0t
Jun 14 at 11:37
@t3chb0t I have updated my question. I hope it is clearer now...
– Thaoden
Jun 14 at 12:02
1
Well, your interface defines a lot more methods that the implementation you show us. Why did you cut out the other methods? Post them all. Edited code is always a bad thing. Don't change anything. Just post it as is.
– t3chb0t
Jun 14 at 12:07
|
show 2 more comments
up vote
1
down vote
favorite
up vote
1
down vote
favorite
I have a web application that allows users to create customs items and later change those items. For eg. a change, the application takes the parameters and performs some basic sanity checks.
If the checks pass, a REST service is called and the result of the response is converted to an ActionResult that can then be further handled by the application.
Is the checks fail, I currently create my own HttpResponseMessage that can then also be converted to an ActionResult:
public class HomeController : Controller
{
private readonly IRoleManagerServiceAccess _access;
private const string TEXT_PLAIN = "text/plain";
public HomeController(IRoleManagerServiceAccess access)
{
_access = access;
}
[HttpPut("[action]/{groupImportId?}")]
public async Task<ActionResult<DisplayGroupDto>> ChangeGroupDescription(string groupImportId, [FromBody] string groupDescription)
{
var serviceResponse = await _access.ChangeGroupDescriptionAsync(groupImportId, groupDescription);
if(serviceResponse.IsSuccessStatusCode)
{
return await serviceResponse.Content.ReadAsAsync<DisplayGroupDto>();
}
return new ContentResult
{
StatusCode = (int?)serviceResponse.StatusCode,
Content = await serviceResponse.Content.ReadAsStringAsync(),
ContentType = TEXT_PLAIN
};
}
}
public sealed class RoleManagerServiceAccess : IRoleManagerServiceAccess
{
private readonly HttpClient _httpClient;
public RoleManagerServiceAccess(HttpClient httpClient)
{
_httpClient = httpClient;
public async Task<HttpResponseMessage> ChangeGroupDescriptionAsync(string groupImportId, string groupDescription)
{
return
changeGroupDescriptionParamsAreInvalid(groupImportId, groupDescription) ??
await changeGroupDescriptionParamsAreValidAsync(groupImportId, groupDescription);
}
private async Task<HttpResponseMessage> changeGroupDescriptionParamsAreValidAsync(string groupImportId, string groupDescription)
{
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Description, groupDescription.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
private static HttpResponseMessage changeGroupDescriptionParamsAreInvalid(string groupImportId, string groupDescription)
{
return groupImportIdIsInvalid(groupImportId)
?? groupDescriptionIsInvalid(groupDescription);
}
private static HttpResponseMessage groupDescriptionIsInvalid(string groupDescription)
{
return groupDescription == null
? new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent("Please provide a non-null value for the group description.") }
: null;
}
private static HttpResponseMessage groupImportIdIsInvalid(string groupImportId)
{
return String.IsNullOrWhiteSpace(groupImportId)
? new HttpResponseMessage { StatusCode = HttpStatusCode.BadRequest, Content = new StringContent("Please provide a non-whitespace groupImportId.") }
: null;
}
public async Task<HttpResponseMessage> ChangeGroupImportIdAsync(string oldGroupImportId, string newGroupImportId)
{
if (String.IsNullOrWhiteSpace(oldGroupImportId))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace value for the old group importId.")
};
}
if (String.IsNullOrWhiteSpace(newGroupImportId))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace value for the new group importId.")
};
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.ImportId, newGroupImportId.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(oldGroupImportId, jsonPatch));
}
public async Task<HttpResponseMessage> CreateGroupAsync(string parentGroupImportId, CreateGroupDto createGroupDto)
{
if (createGroupDto == null)
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-null group for creation")
};
}
if (String.IsNullOrWhiteSpace(createGroupDto.Name))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace groupName.")
};
}
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(createGroupDto.ImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
if (String.IsNullOrWhiteSpace(parentGroupImportId))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace parentGroupImportId.")
};
}
return await _httpClient.PostAsJsonAsync<CreateGroupDto>($"api/group/{parentGroupImportId}", createGroupDto);
}
public async Task<HttpResponseMessage> DeleteGroupAsync(string groupImportId, bool forceDelete)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.DeleteAsync($"api/group/{groupImportId}?forceDelete={forceDelete}");
}
public async Task<HttpResponseMessage> GetGroupAsync(string groupImportId)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.GetAsync($"/api/group/{groupImportId}");
}
public async Task<HttpResponseMessage> RenameGroupAsync(string groupImportId, string groupName)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
if (String.IsNullOrWhiteSpace(groupName))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace value for the group name.")
};
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Name, groupName.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
private static HttpRequestMessage preparePatchRequestMessage(string groupImportId, JsonPatchDocument<CreateGroupDto> jsonPatch)
{
return new HttpRequestMessage
{
Method = HttpMethod.Patch,
RequestUri = new Uri($"api/group/{groupImportId}", UriKind.Relative),
Content = new ObjectContent<JsonPatchDocument<CreateGroupDto>>(jsonPatch, new JsonMediaTypeFormatter())
};
}
public async Task<HttpResponseMessage> SetRulePartsAsync(string groupImportId, List<CreateRulePartDto> ruleParts)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
if (ruleParts == null)
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a list of rule parts.")
};
}
return await _httpClient.PutAsJsonAsync<List<CreateRulePartDto>>($"api/group/{groupImportId}/ruleParts", ruleParts);
}
public async Task<HttpResponseMessage> GetGroupMembersAsync(string groupImportId)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.GetAsync($"api/group/{groupImportId}/members");
}
public async Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members)
{
if (members == null)
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a valid members list")
};
}
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.PutAsJsonAsync<List<string>>($"api/group/{groupImportId}/members", members);
}
}
As you can see, in the RoleManagerServiceAccess code, I make extensive use of the conditional and the null-conditional operators to perform my sanity checks. This allows me to return a HttpResponseMessage both in the check-failing and the check-passing cases. I don't have to throw exceptions that I then would have to handle in the HomeController in order to translate them to an ActionResult but can directly use the response from the RoleManagerServiceAccess, be it a "real" response from the REST service or a "constructed" response because the checks are failing.
Of course, the ChangeGroupDescription is not the only method there is, the sanity checks are mostly the same across all methods.
Now the question is: Is this a good idea? Is it clear what is going on here or does it only help in obfuscating the code's intent?
Thanks for any input!
EDIT
Trying to provide some more context.
The interface looks like this:
public interface IRoleManagerServiceAccess
{
Task<HttpResponseMessage> GetGroupAsync(string groupImportId);
Task<HttpResponseMessage> CreateGroupAsync(string parentGroupImportId, Dto.CreateGroupDto createGroupDto);
Task<HttpResponseMessage> DeleteGroupAsync(string groupImportId, bool forceDelete);
Task<HttpResponseMessage> SetRulePartsAsync(string groupImportId, List<CreateRulePartDto> ruleParts);
Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members);
Task<HttpResponseMessage> RenameGroupAsync(string groupImportId, string groupName);
Task<HttpResponseMessage> ChangeGroupDescriptionAsync(string groupImportId, string groupDescription);
Task<HttpResponseMessage> ChangeGroupImportIdAsync(string oldGroupImportId, string newGroupImportId);
Task<HttpResponseMessage> GetGroupMembersAsync(string groupImportId);
}
Each of these methods gets passed the same parameter groupImportId and possibly, depending on functionality, an additional parameter. In each of these methods, I check if groupImportId is whitespace, if that is the case, I don't need to call the REST service. If an additional parameter is passed, as in the case of ChangeGroupDescription, it is checked for constraints also and if it fails, the REST service is not called.
If all checks for the parameters pass, the service is called with the parameters and the response is passed back to the caller. The service response's status code can indicate a success, in which case I want the HomeController to return the deserialized object of the response, or it can indicate a failure, in which case I want the HomeController to return a HttpResponseMessage with the same StatusCode and Content as the service response's.
In the case of a failing sanity check, I want the HomeController to also return a ContentResult with code 400 and some Content. Previously, I had thrown an ArgumentNullException in the RoleManagerServiceAccess methods; in each method of HomeController, I had to wrap the call to _access.xyz() in a try-catch-block, where I would examine the exception's message and build the ContentResult from it. That lead to nine times the same try-catch-block in HomeController and nine times the same throw new ArgumentNullException() block in RoleManagerServiceAccess. I thought there might be a better way.
Another edit:
I've reverted my code to a version without coalesing and conditional operators. I think it is clearer now what is happening, although I'm still not happy with the overall structure.
public sealed class RoleManagerServiceAccess : IRoleManagerServiceAccess
{
private readonly HttpClient _httpClient;
private const string GROUPIMPORTID_BADREQUEST_MESSAGE = "Please provide a non-whitespace groupImportId.";
public RoleManagerServiceAccess(HttpClient httpClient)
{
_httpClient = httpClient;
}
public async Task<HttpResponseMessage> ChangeGroupDescriptionAsync(string groupImportId, string groupDescription)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (groupDescription == null)
{
return createBadRequestResponse("Please provide a non-null value for the group description.");
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Description, groupDescription.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
public async Task<HttpResponseMessage> ChangeGroupImportIdAsync(string oldGroupImportId, string newGroupImportId)
{
if (String.IsNullOrWhiteSpace(oldGroupImportId))
{
return createBadRequestResponse("Please provide a non-whitespace value for the old group importId.");
}
if (String.IsNullOrWhiteSpace(newGroupImportId))
{
return createBadRequestResponse("Please provide a non-whitespace value for the new group importId.");
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.ImportId, newGroupImportId.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(oldGroupImportId, jsonPatch));
}
public async Task<HttpResponseMessage> CreateGroupAsync(string parentGroupImportId, CreateGroupDto createGroupDto)
{
if (createGroupDto == null)
{
return createBadRequestResponse("Please provide a non-null group for creation");
}
if (String.IsNullOrWhiteSpace(createGroupDto.Name))
{
return createBadRequestResponse("Please provide a non-whitespace groupName.");
}
if (String.IsNullOrWhiteSpace(createGroupDto.ImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (String.IsNullOrWhiteSpace(parentGroupImportId))
{
return createBadRequestResponse("Please provide a non-whitespace parentGroupImportId.");
}
return await _httpClient.PostAsJsonAsync<CreateGroupDto>($"api/group/{parentGroupImportId}", createGroupDto);
}
public async Task<HttpResponseMessage> DeleteGroupAsync(string groupImportId, bool forceDelete)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.DeleteAsync($"api/group/{groupImportId}?forceDelete={forceDelete}");
}
public async Task<HttpResponseMessage> GetGroupAsync(string groupImportId)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.GetAsync($"/api/group/{groupImportId}");
}
public async Task<HttpResponseMessage> RenameGroupAsync(string groupImportId, string groupName)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (String.IsNullOrWhiteSpace(groupName))
{
return createBadRequestResponse("Please provide a non-whitespace value for the group name.");
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Name, groupName.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
private static HttpRequestMessage preparePatchRequestMessage(string groupImportId, JsonPatchDocument<CreateGroupDto> jsonPatch)
{
return new HttpRequestMessage
{
Method = HttpMethod.Patch,
RequestUri = new Uri($"api/group/{groupImportId}", UriKind.Relative),
Content = new ObjectContent<JsonPatchDocument<CreateGroupDto>>(jsonPatch, new JsonMediaTypeFormatter())
};
}
public async Task<HttpResponseMessage> SetRulePartsAsync(string groupImportId, List<CreateRulePartDto> ruleParts)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (ruleParts == null)
{
return createBadRequestResponse("Please provide a list of rule parts.");
}
return await _httpClient.PutAsJsonAsync<List<CreateRulePartDto>>($"api/group/{groupImportId}/ruleParts", ruleParts);
}
public async Task<HttpResponseMessage> GetGroupMembersAsync(string groupImportId)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.GetAsync($"api/group/{groupImportId}/members");
}
public async Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members)
{
if (members == null)
{
return createBadRequestResponse("Please provide a valid members list");
}
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.PutAsJsonAsync<List<string>>($"api/group/{groupImportId}/members", members);
}
private static HttpResponseMessage createBadRequestResponse(string message) =>
new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent(message) };
}
c# rest null asp.net-web-api
asked Jun 14 at 9:46
Thaoden
1266
I have a web application that allows users to create customs items and later change those items. For eg. a change, the application takes the parameters and performs some basic sanity checks.
If the checks pass, a REST service is called and the result of the response is converted to an ActionResult that can then be further handled by the application.
Is the checks fail, I currently create my own HttpResponseMessage that can then also be converted to an ActionResult:
public class HomeController : Controller
{
private readonly IRoleManagerServiceAccess _access;
private const string TEXT_PLAIN = "text/plain";
public HomeController(IRoleManagerServiceAccess access)
{
_access = access;
}
[HttpPut("[action]/{groupImportId?}")]
public async Task<ActionResult<DisplayGroupDto>> ChangeGroupDescription(string groupImportId, [FromBody] string groupDescription)
{
var serviceResponse = await _access.ChangeGroupDescriptionAsync(groupImportId, groupDescription);
if(serviceResponse.IsSuccessStatusCode)
{
return await serviceResponse.Content.ReadAsAsync<DisplayGroupDto>();
}
return new ContentResult
{
StatusCode = (int?)serviceResponse.StatusCode,
Content = await serviceResponse.Content.ReadAsStringAsync(),
ContentType = TEXT_PLAIN
};
}
}
public sealed class RoleManagerServiceAccess : IRoleManagerServiceAccess
{
private readonly HttpClient _httpClient;
public RoleManagerServiceAccess(HttpClient httpClient)
{
_httpClient = httpClient;
public async Task<HttpResponseMessage> ChangeGroupDescriptionAsync(string groupImportId, string groupDescription)
{
return
changeGroupDescriptionParamsAreInvalid(groupImportId, groupDescription) ??
await changeGroupDescriptionParamsAreValidAsync(groupImportId, groupDescription);
}
private async Task<HttpResponseMessage> changeGroupDescriptionParamsAreValidAsync(string groupImportId, string groupDescription)
{
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Description, groupDescription.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
private static HttpResponseMessage changeGroupDescriptionParamsAreInvalid(string groupImportId, string groupDescription)
{
return groupImportIdIsInvalid(groupImportId)
?? groupDescriptionIsInvalid(groupDescription);
}
private static HttpResponseMessage groupDescriptionIsInvalid(string groupDescription)
{
return groupDescription == null
? new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent("Please provide a non-null value for the group description.") }
: null;
}
private static HttpResponseMessage groupImportIdIsInvalid(string groupImportId)
{
return String.IsNullOrWhiteSpace(groupImportId)
? new HttpResponseMessage { StatusCode = HttpStatusCode.BadRequest, Content = new StringContent("Please provide a non-whitespace groupImportId.") }
: null;
}
public async Task<HttpResponseMessage> ChangeGroupImportIdAsync(string oldGroupImportId, string newGroupImportId)
{
if (String.IsNullOrWhiteSpace(oldGroupImportId))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace value for the old group importId.")
};
}
if (String.IsNullOrWhiteSpace(newGroupImportId))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace value for the new group importId.")
};
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.ImportId, newGroupImportId.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(oldGroupImportId, jsonPatch));
}
public async Task<HttpResponseMessage> CreateGroupAsync(string parentGroupImportId, CreateGroupDto createGroupDto)
{
if (createGroupDto == null)
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-null group for creation")
};
}
if (String.IsNullOrWhiteSpace(createGroupDto.Name))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace groupName.")
};
}
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(createGroupDto.ImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
if (String.IsNullOrWhiteSpace(parentGroupImportId))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace parentGroupImportId.")
};
}
return await _httpClient.PostAsJsonAsync<CreateGroupDto>($"api/group/{parentGroupImportId}", createGroupDto);
}
public async Task<HttpResponseMessage> DeleteGroupAsync(string groupImportId, bool forceDelete)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.DeleteAsync($"api/group/{groupImportId}?forceDelete={forceDelete}");
}
public async Task<HttpResponseMessage> GetGroupAsync(string groupImportId)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.GetAsync($"/api/group/{groupImportId}");
}
public async Task<HttpResponseMessage> RenameGroupAsync(string groupImportId, string groupName)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
if (String.IsNullOrWhiteSpace(groupName))
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a non-whitespace value for the group name.")
};
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Name, groupName.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
private static HttpRequestMessage preparePatchRequestMessage(string groupImportId, JsonPatchDocument<CreateGroupDto> jsonPatch)
{
return new HttpRequestMessage
{
Method = HttpMethod.Patch,
RequestUri = new Uri($"api/group/{groupImportId}", UriKind.Relative),
Content = new ObjectContent<JsonPatchDocument<CreateGroupDto>>(jsonPatch, new JsonMediaTypeFormatter())
};
}
public async Task<HttpResponseMessage> SetRulePartsAsync(string groupImportId, List<CreateRulePartDto> ruleParts)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
if (ruleParts == null)
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a list of rule parts.")
};
}
return await _httpClient.PutAsJsonAsync<List<CreateRulePartDto>>($"api/group/{groupImportId}/ruleParts", ruleParts);
}
public async Task<HttpResponseMessage> GetGroupMembersAsync(string groupImportId)
{
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.GetAsync($"api/group/{groupImportId}/members");
}
public async Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members)
{
if (members == null)
{
return new HttpResponseMessage
{
StatusCode = HttpStatusCode.BadRequest,
Content = new StringContent("Please provide a valid members list")
};
}
HttpResponseMessage groupImportIdIsValidResponse = groupImportIdIsInvalid(groupImportId);
if (groupImportIdIsValidResponse != null)
{
return groupImportIdIsValidResponse;
}
return await _httpClient.PutAsJsonAsync<List<string>>($"api/group/{groupImportId}/members", members);
}
}
As you can see, in the RoleManagerServiceAccess code, I make extensive use of the conditional and the null-conditional operators to perform my sanity checks. This allows me to return a HttpResponseMessage both in the check-failing and the check-passing cases. I don't have to throw exceptions that I then would have to handle in the HomeController in order to translate them to an ActionResult but can directly use the response from the RoleManagerServiceAccess, be it a "real" response from the REST service or a "constructed" response because the checks are failing.
Of course, the ChangeGroupDescription is not the only method there is, the sanity checks are mostly the same across all methods.
Now the question is: Is this a good idea? Is it clear what is going on here or does it only help in obfuscating the code's intent?
Thanks for any input!
EDIT
Trying to provide some more context.
The interface looks like this:
public interface IRoleManagerServiceAccess
{
Task<HttpResponseMessage> GetGroupAsync(string groupImportId);
Task<HttpResponseMessage> CreateGroupAsync(string parentGroupImportId, Dto.CreateGroupDto createGroupDto);
Task<HttpResponseMessage> DeleteGroupAsync(string groupImportId, bool forceDelete);
Task<HttpResponseMessage> SetRulePartsAsync(string groupImportId, List<CreateRulePartDto> ruleParts);
Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members);
Task<HttpResponseMessage> RenameGroupAsync(string groupImportId, string groupName);
Task<HttpResponseMessage> ChangeGroupDescriptionAsync(string groupImportId, string groupDescription);
Task<HttpResponseMessage> ChangeGroupImportIdAsync(string oldGroupImportId, string newGroupImportId);
Task<HttpResponseMessage> GetGroupMembersAsync(string groupImportId);
}
Each of these methods gets passed the same parameter groupImportId and possibly, depending on functionality, an additional parameter. In each of these methods, I check if groupImportId is whitespace, if that is the case, I don't need to call the REST service. If an additional parameter is passed, as in the case of ChangeGroupDescription, it is checked for constraints also and if it fails, the REST service is not called.
If all checks for the parameters pass, the service is called with the parameters and the response is passed back to the caller. The service response's status code can indicate a success, in which case I want the HomeController to return the deserialized object of the response, or it can indicate a failure, in which case I want the HomeController to return a HttpResponseMessage with the same StatusCode and Content as the service response's.
In the case of a failing sanity check, I want the HomeController to also return a ContentResult with code 400 and some Content. Previously, I had thrown an ArgumentNullException in the RoleManagerServiceAccess methods; in each method of HomeController, I had to wrap the call to _access.xyz() in a try-catch-block, where I would examine the exception's message and build the ContentResult from it. That lead to nine times the same try-catch-block in HomeController and nine times the same throw new ArgumentNullException() block in RoleManagerServiceAccess. I thought there might be a better way.
Another edit:
I've reverted my code to a version without coalesing and conditional operators. I think it is clearer now what is happening, although I'm still not happy with the overall structure.
public sealed class RoleManagerServiceAccess : IRoleManagerServiceAccess
{
private readonly HttpClient _httpClient;
private const string GROUPIMPORTID_BADREQUEST_MESSAGE = "Please provide a non-whitespace groupImportId.";
public RoleManagerServiceAccess(HttpClient httpClient)
{
_httpClient = httpClient;
}
public async Task<HttpResponseMessage> ChangeGroupDescriptionAsync(string groupImportId, string groupDescription)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (groupDescription == null)
{
return createBadRequestResponse("Please provide a non-null value for the group description.");
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Description, groupDescription.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
public async Task<HttpResponseMessage> ChangeGroupImportIdAsync(string oldGroupImportId, string newGroupImportId)
{
if (String.IsNullOrWhiteSpace(oldGroupImportId))
{
return createBadRequestResponse("Please provide a non-whitespace value for the old group importId.");
}
if (String.IsNullOrWhiteSpace(newGroupImportId))
{
return createBadRequestResponse("Please provide a non-whitespace value for the new group importId.");
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.ImportId, newGroupImportId.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(oldGroupImportId, jsonPatch));
}
public async Task<HttpResponseMessage> CreateGroupAsync(string parentGroupImportId, CreateGroupDto createGroupDto)
{
if (createGroupDto == null)
{
return createBadRequestResponse("Please provide a non-null group for creation");
}
if (String.IsNullOrWhiteSpace(createGroupDto.Name))
{
return createBadRequestResponse("Please provide a non-whitespace groupName.");
}
if (String.IsNullOrWhiteSpace(createGroupDto.ImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (String.IsNullOrWhiteSpace(parentGroupImportId))
{
return createBadRequestResponse("Please provide a non-whitespace parentGroupImportId.");
}
return await _httpClient.PostAsJsonAsync<CreateGroupDto>($"api/group/{parentGroupImportId}", createGroupDto);
}
public async Task<HttpResponseMessage> DeleteGroupAsync(string groupImportId, bool forceDelete)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.DeleteAsync($"api/group/{groupImportId}?forceDelete={forceDelete}");
}
public async Task<HttpResponseMessage> GetGroupAsync(string groupImportId)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.GetAsync($"/api/group/{groupImportId}");
}
public async Task<HttpResponseMessage> RenameGroupAsync(string groupImportId, string groupName)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (String.IsNullOrWhiteSpace(groupName))
{
return createBadRequestResponse("Please provide a non-whitespace value for the group name.");
}
var jsonPatch = new JsonPatchDocument<CreateGroupDto>();
jsonPatch.Replace<string>(g => g.Name, groupName.Trim());
return await _httpClient.SendAsync(preparePatchRequestMessage(groupImportId, jsonPatch));
}
private static HttpRequestMessage preparePatchRequestMessage(string groupImportId, JsonPatchDocument<CreateGroupDto> jsonPatch)
{
return new HttpRequestMessage
{
Method = HttpMethod.Patch,
RequestUri = new Uri($"api/group/{groupImportId}", UriKind.Relative),
Content = new ObjectContent<JsonPatchDocument<CreateGroupDto>>(jsonPatch, new JsonMediaTypeFormatter())
};
}
public async Task<HttpResponseMessage> SetRulePartsAsync(string groupImportId, List<CreateRulePartDto> ruleParts)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
if (ruleParts == null)
{
return createBadRequestResponse("Please provide a list of rule parts.");
}
return await _httpClient.PutAsJsonAsync<List<CreateRulePartDto>>($"api/group/{groupImportId}/ruleParts", ruleParts);
}
public async Task<HttpResponseMessage> GetGroupMembersAsync(string groupImportId)
{
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.GetAsync($"api/group/{groupImportId}/members");
}
public async Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members)
{
if (members == null)
{
return createBadRequestResponse("Please provide a valid members list");
}
if (String.IsNullOrWhiteSpace(groupImportId))
{
return createBadRequestResponse(GROUPIMPORTID_BADREQUEST_MESSAGE);
}
return await _httpClient.PutAsJsonAsync<List<string>>($"api/group/{groupImportId}/members", members);
}
private static HttpResponseMessage createBadRequestResponse(string message) =>
new HttpResponseMessage(HttpStatusCode.BadRequest) { Content = new StringContent(message) };
}
c# rest null asp.net-web-api
c# rest null asp.net-web-api
asked Jun 14 at 9:46
Thaoden
1266
asked Jun 14 at 9:46
Thaoden
1266
edited Jun 14 at 14:26
asked Jun 14 at 9:46
Thaoden
1266
asked Jun 14 at 9:46
Thaoden
1266
asked Jun 14 at 9:46
Thaoden
1266
1266
1
This is super-confusing! You callchangeGroupDescriptionParamsAreInvalidas if you already knew that parameters are invalid then you try to call the other API because your guess about invalid parameters failed. This is horrible :-| You have a single public API. Just check the parameters and handle it accordingly instead of trying to guess it in several chained methods. This is not at all clever.
– t3chb0t
Jun 14 at 11:11
@t3chb0t Thanks for your input. I'm trying to not clutter my code with the same sanity checks all over the place and not to have duplicate code between raising an exception and handling that same exception. Any hints how to get there?
– Thaoden
Jun 14 at 11:30
1
You have to provide more code if you want us to see patterns. Seeing only this I can only say what I've already said.
– t3chb0t
Jun 14 at 11:37
@t3chb0t I have updated my question. I hope it is clearer now...
– Thaoden
Jun 14 at 12:02
1
Well, your interface defines a lot more methods that the implementation you show us. Why did you cut out the other methods? Post them all. Edited code is always a bad thing. Don't change anything. Just post it as is.
– t3chb0t
Jun 14 at 12:07
|
show 2 more comments
1
This is super-confusing! You callchangeGroupDescriptionParamsAreInvalidas if you already knew that parameters are invalid then you try to call the other API because your guess about invalid parameters failed. This is horrible :-| You have a single public API. Just check the parameters and handle it accordingly instead of trying to guess it in several chained methods. This is not at all clever.
– t3chb0t
Jun 14 at 11:11
@t3chb0t Thanks for your input. I'm trying to not clutter my code with the same sanity checks all over the place and not to have duplicate code between raising an exception and handling that same exception. Any hints how to get there?
– Thaoden
Jun 14 at 11:30
1
You have to provide more code if you want us to see patterns. Seeing only this I can only say what I've already said.
– t3chb0t
Jun 14 at 11:37
@t3chb0t I have updated my question. I hope it is clearer now...
– Thaoden
Jun 14 at 12:02
1
Well, your interface defines a lot more methods that the implementation you show us. Why did you cut out the other methods? Post them all. Edited code is always a bad thing. Don't change anything. Just post it as is.
– t3chb0t
Jun 14 at 12:07
1
1
This is super-confusing! You call
changeGroupDescriptionParamsAreInvalid as if you already knew that parameters are invalid then you try to call the other API because your guess about invalid parameters failed. This is horrible :-| You have a single public API. Just check the parameters and handle it accordingly instead of trying to guess it in several chained methods. This is not at all clever.– t3chb0t
Jun 14 at 11:11
This is super-confusing! You call
changeGroupDescriptionParamsAreInvalid as if you already knew that parameters are invalid then you try to call the other API because your guess about invalid parameters failed. This is horrible :-| You have a single public API. Just check the parameters and handle it accordingly instead of trying to guess it in several chained methods. This is not at all clever.– t3chb0t
Jun 14 at 11:11
@t3chb0t Thanks for your input. I'm trying to not clutter my code with the same sanity checks all over the place and not to have duplicate code between raising an exception and handling that same exception. Any hints how to get there?
– Thaoden
Jun 14 at 11:30
@t3chb0t Thanks for your input. I'm trying to not clutter my code with the same sanity checks all over the place and not to have duplicate code between raising an exception and handling that same exception. Any hints how to get there?
– Thaoden
Jun 14 at 11:30
1
1
You have to provide more code if you want us to see patterns. Seeing only this I can only say what I've already said.
– t3chb0t
Jun 14 at 11:37
You have to provide more code if you want us to see patterns. Seeing only this I can only say what I've already said.
– t3chb0t
Jun 14 at 11:37
@t3chb0t I have updated my question. I hope it is clearer now...
– Thaoden
Jun 14 at 12:02
@t3chb0t I have updated my question. I hope it is clearer now...
– Thaoden
Jun 14 at 12:02
1
1
Well, your interface defines a lot more methods that the implementation you show us. Why did you cut out the other methods? Post them all. Edited code is always a bad thing. Don't change anything. Just post it as is.
– t3chb0t
Jun 14 at 12:07
Well, your interface defines a lot more methods that the implementation you show us. Why did you cut out the other methods? Post them all. Edited code is always a bad thing. Don't change anything. Just post it as is.
– t3chb0t
Jun 14 at 12:07
|
show 2 more comments
1 Answer
1
active
oldest
votes
up vote
0
down vote
I think your latest version is much better. When writing APIs you have to validate the input parameters - that's normal but try to keep it short. A one-line comment after the validation section helps.
Try shorter names, it can really make a difference.
public async Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members)
{
if (members == null)
return BadRequest("Please provide a valid members list");
if (string.IsNullOrWhiteSpace(groupImportId))
return BadRequest(GROUPIMPORTID_BADREQUEST_MESSAGE);
// Set group members
return await _httpClient.PutAsJsonAsync<List<string>>($"api/group/{groupImportId}/members", members);
}
add a comment |
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
0
down vote
I think your latest version is much better. When writing APIs you have to validate the input parameters - that's normal but try to keep it short. A one-line comment after the validation section helps.
Try shorter names, it can really make a difference.
public async Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members)
{
if (members == null)
return BadRequest("Please provide a valid members list");
if (string.IsNullOrWhiteSpace(groupImportId))
return BadRequest(GROUPIMPORTID_BADREQUEST_MESSAGE);
// Set group members
return await _httpClient.PutAsJsonAsync<List<string>>($"api/group/{groupImportId}/members", members);
}
add a comment |
up vote
0
down vote
I think your latest version is much better. When writing APIs you have to validate the input parameters - that's normal but try to keep it short. A one-line comment after the validation section helps.
Try shorter names, it can really make a difference.
public async Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members)
{
if (members == null)
return BadRequest("Please provide a valid members list");
if (string.IsNullOrWhiteSpace(groupImportId))
return BadRequest(GROUPIMPORTID_BADREQUEST_MESSAGE);
// Set group members
return await _httpClient.PutAsJsonAsync<List<string>>($"api/group/{groupImportId}/members", members);
}
add a comment |
up vote
0
down vote
up vote
0
down vote
I think your latest version is much better. When writing APIs you have to validate the input parameters - that's normal but try to keep it short. A one-line comment after the validation section helps.
Try shorter names, it can really make a difference.
public async Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members)
{
if (members == null)
return BadRequest("Please provide a valid members list");
if (string.IsNullOrWhiteSpace(groupImportId))
return BadRequest(GROUPIMPORTID_BADREQUEST_MESSAGE);
// Set group members
return await _httpClient.PutAsJsonAsync<List<string>>($"api/group/{groupImportId}/members", members);
}
I think your latest version is much better. When writing APIs you have to validate the input parameters - that's normal but try to keep it short. A one-line comment after the validation section helps.
Try shorter names, it can really make a difference.
public async Task<HttpResponseMessage> SetGroupMembersAsync(string groupImportId, List<string> members)
{
if (members == null)
return BadRequest("Please provide a valid members list");
if (string.IsNullOrWhiteSpace(groupImportId))
return BadRequest(GROUPIMPORTID_BADREQUEST_MESSAGE);
// Set group members
return await _httpClient.PutAsJsonAsync<List<string>>($"api/group/{groupImportId}/members", members);
}
answered Jun 25 at 15:57
user106794
add a comment |
add a comment |
draft saved
draft discarded
draft saved
draft discarded
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f196481%2fusing-coalescing-and-conditional-operators-to-check-api-arguments%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
1
This is super-confusing! You call
changeGroupDescriptionParamsAreInvalidas if you already knew that parameters are invalid then you try to call the other API because your guess about invalid parameters failed. This is horrible :-| You have a single public API. Just check the parameters and handle it accordingly instead of trying to guess it in several chained methods. This is not at all clever.– t3chb0t
Jun 14 at 11:11
@t3chb0t Thanks for your input. I'm trying to not clutter my code with the same sanity checks all over the place and not to have duplicate code between raising an exception and handling that same exception. Any hints how to get there?
– Thaoden
Jun 14 at 11:30
1
You have to provide more code if you want us to see patterns. Seeing only this I can only say what I've already said.
– t3chb0t
Jun 14 at 11:37
@t3chb0t I have updated my question. I hope it is clearer now...
– Thaoden
Jun 14 at 12:02
1
Well, your interface defines a lot more methods that the implementation you show us. Why did you cut out the other methods? Post them all. Edited code is always a bad thing. Don't change anything. Just post it as is.
– t3chb0t
Jun 14 at 12:07