Submit form data to server side

Question

I have this method here called SubmitProc which submits form data that the user filled out to the server side and does a check if the response to the server goes through. If it's fine, great. If not, return the status error from the server. The data actually gets sent to SharePoint (not sure if it's politically correct to say it goes to server side if it goes to SP) but that's the idea.

If an exceptions results, then check if the files upload match the current procedure file count. If yes, do nothing, otherwise there is an error.

public async Task<string> SubmitProc(PFormData formData)
{
 Proj.Repositories.Models.External.Proc proc = formData.ProcForm;
 try
 {
 string url = Config.BaseAPIUrl;
 url = url + "AddPProc";
 string authToken = await Authentication.GetAuthToken();
 _client.DefaultRequestHeaders.Accept.Add (new System.Net.Http.Headers.MediaTypeWithQualityHeaderValue ("application/json"));
 _clientHandler.CookieContainer.Add (new Uri (url), new System.Net.Cookie ("FedAuth", authToken));
 Proj.Repositories.Models.External.ProcResponse serviceResponse = await submitForm (url, proc).ConfigureAwait (false);
 if (serviceResponse.StatusCode == 200)
 return null;
 else
 return serviceResponse.StatusDescription;
 }
 catch (WebException ex)
 {
 }
 catch (Exception ex)
 {
 }
 try
 {
 if (proc.File != null)
 {
 int filesCount = await getUploadFilesCount ("Foo", proc.UniqueId);
 if (proc.File.Count == filesCount)
 return null;
 else
 return "Encountered error uploading attachments.";
 }
 }
 catch (Exception ex)
 {
 }
 return "Encountered unknown error.";
}

Where getUploadFilesCount is:

private async Task<int> getUploadFilesCount(string type, string guidString)
{
 try
 {
 string url = Config.BaseAPIUrl;
 url = url + "/VerifyUpload/" + type + "/" + guidString;// + "/" + {*procDate};
 string authToken = await Authentication.GetAuthToken ();
 _client.DefaultRequestHeaders.Accept.Add (new System.Net.Http.Headers.MediaTypeWithQualityHeaderValue ("application/json"));
 _clientHandler.CookieContainer.Add (new Uri (url), new System.Net.Cookie ("FedAuth", authToken));
 HttpRequestMessage request = new HttpRequestMessage (HttpMethod.Get, url);
 HttpResponseMessage response = await _client.SendAsync (request);
 Proj.Repositories.Models.External.ProcResponse procResponse = JsonConvert.DeserializeObject<Proj.Repositories.Models.External.ProcResponse> (await response.Content.ReadAsStringAsync ());
 if(procResponse != null)
 return procResponse.FilesCount;
 return 0;
 }
 catch(Exception)
 {
 }
 return 0;
}

It's been working so far but the code above looks terrible. I'm maintaining this project and I just looked at this code and it definitely looks like it needs to be refactored but I'm afraid of breaking it. Any recommendations or advice on condensing this code or refactor it?

This code is currently inside a Xamarin.iOS App.

Answer 1

I don't see any rough mistake with your code, but I would refactor some parts of it.

There are a lot of magic string constants like "AddPProc", "VerifyUpload", "FedAuth". They should be defined as named constants:

private const string AddPProc = "AddPProc";

This code

string url = Config.BaseAPIUrl;
url = url + "AddPProc";

can be replaced with

var url = Config.BaseAPIUrl + AddPProc;

The same thing with the second method.

In both methods there are fully qualified types. It is hard to read this code. For example, instead of System.Net.Http.Headers.MediaTypeWithQualityHeaderValue place using System.Net.Http.Headers; at the start of the file and use MediaTypeWithQualityHeaderValue without qualification. Especially it is recommended for this line:

Proj.Repositories.Models.External.ProcResponse procResponse = JsonConvert.DeserializeObject<Proj.Repositories.Models.External.ProcResponse> (await response.Content.ReadAsStringAsync ());

In this code

if (serviceResponse.StatusCode == 200)
 return null;
else
 return serviceResponse.StatusDescription;

you don't need else keyword. You can even replace this if-else with ternary operator and single return:

return serviceResponse.StatusCode == 200
 ? null
 : serviceResponse.StatusDescription;

getUploadFilesCount is bad name according to C# naming guidelines. Method's name should start with uppercase letter.

This code

if (procResponse != null)
 return procResponse.FilesCount;
return 0;

can be replaced with

return procResponse?.FilesCount ?? 0;

Also it is not good to return string to show that something went wrong. Let exceptions be thrown, don't swallow them. Code that calls these methods should take care about exceptions.

And I don't understand why a space is placed before each ( in methods calls.