2
\$\begingroup\$

I would like to know how to know how can I optimize my current Post method , I initialize object where I am creating list of json items. Also how can I reduce many if and if else statments? For this post and get methods I am using swagger , is it possible to declare exceptions objects somewhere globaly? What kind of good practice there is regarding swagger objects, maybe I have to declare somewhere as json structure?

[HttpPost]
        public IHttpActionResult Post([FromBody] SpecCaseRequest specCaseRequest)
        {
            var specCase= new SpecCase();

            specCase.CopyFromRequest(specCaseRequest);

            ExceptionResponse[] data = new ExceptionResponse[]
            {
                new ExceptionResponse { Id = 1, Message = "some text", ReturnCode = HttpStatusCode.BadRequest },
                new ExceptionResponse { Id = 2, Message = "some text", ReturnCode = HttpStatusCode.BadRequest },
                new ExceptionResponse { Id = 3, Message = "some text", ReturnCode = HttpStatusCode.Created },
                new ExceptionResponse { Id = 4, Message = "some text", ReturnCode = HttpStatusCode.BadRequest },
                new ExceptionResponse { Id = 5, Message = "some text", ReturnCode = HttpStatusCode.BadRequest },
                new ExceptionResponse { Id = 7, Message = "some text", ReturnCode = HttpStatusCode.BadRequest },
            };

            string json = JsonConvert.SerializeObject(data, Formatting.Indented);

            JArray jo = JArray.Parse(json);
            HttpResponseMessage responseMsg2 = new HttpResponseMessage(HttpStatusCode.BadRequest);   

            if (specCase.Name == "")
            {
                JObject match = jo.Values<JObject>()
                    .Where(m => m["Id"].Value<int>() == 2)
                    .FirstOrDefault();
                responseMsg2.Headers.Location = new Uri(Url.Link("DefaultApi", new { controller = "SpecCases", id = specCase.Id }));
                responseMsg2.Content = new StringContent(match.ToString(), Encoding.UTF8, "application/json");
                responseMsg2.Content.ReadAsStringAsync();
                IHttpActionResult response2 = ResponseMessage(responseMsg2);
                return response2;
            }
            else if (specCase.Name == null)
            {
                JObject match = jo.Values<JObject>()
                    .Where(m => m["Id"].Value<int>() == 1)
                    .FirstOrDefault();
                responseMsg2.Headers.Location = new Uri(Url.Link("DefaultApi", new { controller = "SpecCases", id = specCase.Id }));
                responseMsg2.Content = new StringContent(match.ToString(), Encoding.UTF8, "application/json");
                responseMsg2.Content.ReadAsStringAsync();
                IHttpActionResult response2 = ResponseMessage(responseMsg2);
                return response2;
            }
            else if (specCase.Signatories.Select(x => x.SignatoryId == "" && x.Type == SignatoryType.Company).FirstOrDefault())
            {
                JObject match = jo.Values<JObject>()
                    .Where(m => m["Id"].Value<int>() == 4)
                    .FirstOrDefault();
                responseMsg2.Headers.Location = new Uri(Url.Link("DefaultApi", new { controller = "SpecCases", id = specCase.Id }));
                responseMsg2.Content = new StringContent(match.ToString(), Encoding.UTF8, "application/json");
                responseMsg2.Content.ReadAsStringAsync();
                IHttpActionResult response2 = ResponseMessage(responseMsg2);
                return response2;
            }
            else if (specCaseRequest.Signatories.Select(x => x.Type != SignatoryType.Company).FirstOrDefault()) 
            {
                JObject match = jo.Values<JObject>()
                    .Where(m => m["Id"].Value<int>() == 8)
                    .FirstOrDefault();
                responseMsg2.Headers.Location = new Uri(Url.Link("DefaultApi", new { controller = "SpecCases", id = specCase.Id }));
                responseMsg2.Content = new StringContent(match.ToString(), Encoding.UTF8, "application/json");
                responseMsg2.Content.ReadAsStringAsync();
                IHttpActionResult response2 = ResponseMessage(responseMsg2);
                return response2;
            }
            else if (specCaseRequest.Signatories.Select(x => x.Id == "").FirstOrDefault()) 
            {
                JObject match = jo.Values<JObject>()
                    .Where(m => m["Id"].Value<int>() == 9)
                    .FirstOrDefault();
                responseMsg2.Headers.Location = new Uri(Url.Link("DefaultApi", new { controller = "SpecCases", id = specCase.Id }));
                responseMsg2.Content = new StringContent(match.ToString(), Encoding.UTF8, "application/json");
                responseMsg2.Content.ReadAsStringAsync();
                IHttpActionResult response2 = ResponseMessage(responseMsg2);
                return response2;
            }
            else if (specCase.Signatories.Select(x => x.SignatoryId == null).FirstOrDefault())
            {
                JObject match = jo.Values<JObject>()
                    .Where(m => m["Id"].Value<int>() == 5)
                    .FirstOrDefault();
                responseMsg2.Headers.Location = new Uri(Url.Link("DefaultApi", new { controller = "SpecCases", id = specCase.Id }));
                responseMsg2.Content = new StringContent(match.ToString(), Encoding.UTF8, "application/json");
                responseMsg2.Content.ReadAsStringAsync();
                IHttpActionResult response2 = ResponseMessage(responseMsg2);
                return response2;
            }
            else if (specCase.Signatories.Select(x => x.SignatoryId == "").FirstOrDefault())
            {
                JObject match = jo.Values<JObject>()
                    .Where(m => m["Id"].Value<int>() == 7)
                    .FirstOrDefault();
                responseMsg2.Headers.Location = new Uri(Url.Link("DefaultApi", new { controller = "SpecCases", id = specCase.Id }));
                responseMsg2.Content = new StringContent(match.ToString(), Encoding.UTF8, "application/json");
                responseMsg2.Content.ReadAsStringAsync();
                IHttpActionResult response2 = ResponseMessage(responseMsg2);
                return response2;
            }

            try
            {
                var specCaseRequest= unitOfWork.GetSigningCaseRepository();
                specCaseRequest.Add(specCase);
                unitOfWork.Save();
            }
            catch (Exception e)
            {
                Log.Error(e.Message);
                return InternalServerError();
            }

            JObject match1 = jo.Values<JObject>()
                    .Where(m => m["Id"].Value<int>() == 3)
                    .FirstOrDefault();

            HttpResponseMessage responseMsg = new HttpResponseMessage(HttpStatusCode.Created);
            responseMsg.Headers.Location = new Uri(Url.Link("DefaultApi", new { controller = "SpecCases", id = specCase.Id }));
            responseMsg.Content = new StringContent(match1.ToString(), Encoding.UTF8, "application/json");
            responseMsg.Content.ReadAsStringAsync();
            IHttpActionResult response = ResponseMessage(responseMsg);

            return response;
        }
\$\endgroup\$
  • 2
    \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. You may also want to read How to get the best value out of Code Review - Asking Questions. \$\endgroup\$ – Phrancis Jan 24 '17 at 7:10
  • \$\begingroup\$ @Phrancis Thank you, I already changed Title \$\endgroup\$ – BinaryTie Jan 24 '17 at 8:07
3
\$\begingroup\$

Instead of combining Where + FirstOrDefault you can simply call FirstOrDefault as it accepts predicates as well example:

JObject match1 = jo.Values<JObject>()
    .Where(m => m["Id"].Value<int>() == 3)
    .FirstOrDefault();

Like this:

JObject match1 = jo
    .Values<JObject>()
    .FirstOrDefault(m => m["Id"].Value<int>() == 3);

Next I would suggest to create a dictionary which contains all of your if statements as keys and the values will be they're respective values 1,2,4,5...

This will allow you to make your method really short:

public IHttpActionResult Post(SpecCaseRequest specCaseRequest)
{
    var specCase = new SpecCase();

    specCase.CopyFromRequest(specCaseRequest);

    ExceptionResponse[] data = new ExceptionResponse[]
    {
        new ExceptionResponse { Id = 1, Message = "some text", ReturnCode = HttpStatusCode.BadRequest },
        new ExceptionResponse { Id = 2, Message = "some text", ReturnCode = HttpStatusCode.BadRequest },
        new ExceptionResponse { Id = 3, Message = "some text", ReturnCode = HttpStatusCode.Created },
        new ExceptionResponse { Id = 4, Message = "some text", ReturnCode = HttpStatusCode.BadRequest },
        new ExceptionResponse { Id = 5, Message = "some text", ReturnCode = HttpStatusCode.BadRequest },
        new ExceptionResponse { Id = 7, Message = "some text", ReturnCode = HttpStatusCode.BadRequest },
    };

    string json = JsonConvert.SerializeObject(data, Formatting.Indented);

    JArray jo = JArray.Parse(json);
    var badRequests = new Dictionary<Func<bool>, int?>
    {
        [() => specCase.Name == null] = 1,
        [() => specCase.Name == ""] = 2,
        [() => specCase.Signatories.Select(x => x.SignatoryId == "" && x.Type == SignatoryType.Company)
            .FirstOrDefault()] = 4,
        [() => specCase.Signatories.Select(x => x.SignatoryId == null).FirstOrDefault()] = 5,
        [() => specCase.Signatories.Select(x => x.SignatoryId == "").FirstOrDefault()] = 7,
        [() => specCaseRequest.Signatories.Select(x => x.Type != SignatoryType.Company).FirstOrDefault()] = 8,
        [() => specCaseRequest.Signatories.Select(x => x.Id == "").FirstOrDefault()] = 9,
    };
    HttpResponseMessage responseMsg;
    JObject match;
    int? value = badRequests.FirstOrDefault(kvp => kvp.Key.Invoke()).Value;
    if (value != null)
    {
        match = jo
            .Values<JObject>()
            .FirstOrDefault(m => m["Id"].Value<int>() == value);
        responseMsg = new HttpResponseMessage(HttpStatusCode.BadRequest);
    }
    else
    {
        match = jo
            .Values<JObject>()
            .FirstOrDefault(m => m["Id"].Value<int>() == 3);
        responseMsg = new HttpResponseMessage(HttpStatusCode.Created);
    }

    responseMsg.Headers.Location = new Uri(Url.Link("DefaultApi", new { controller = "SpecCases", id = specCase.Id }));
    responseMsg.Content = new StringContent(match.ToString(), Encoding.UTF8, "application/json");
    responseMsg.Content.ReadAsStringAsync();
    IHttpActionResult response = ResponseMessage(responseMsg);

    return response;
}
\$\endgroup\$
  • \$\begingroup\$ but how it is checking in Dictionary? Because when I am debugging I don't see how he is going into dictionary , he just return value after that line. Is he going inside this variable and goes thru all lines and checking statement when it is true? Please explain how does that Dictionary function works? \$\endgroup\$ – BinaryTie Jan 24 '17 at 14:36
  • \$\begingroup\$ Yes it locates the First element in the dictionary that has value of true and saves the integer value of that element in int? value. \$\endgroup\$ – Denis Jan 24 '17 at 14:42
  • \$\begingroup\$ the func for a key is the most wicked dictionary key I've ever seen :-] \$\endgroup\$ – t3chb0t Jan 24 '17 at 15:28
  • \$\begingroup\$ @t3chb0t Haha It could've been the other way around but it makes more sense to me like this. \$\endgroup\$ – Denis Jan 24 '17 at 15:41
  • \$\begingroup\$ @BinaryTie if there is anything unclear feel free to ask me. \$\endgroup\$ – Denis Jan 24 '17 at 17:15
2
\$\begingroup\$

This one method is 120+ lines long -- and this is just one method in your controller! That alone should have informed you that you should extract this to a separate class. Keep your controller lean, which is easy to achieve with (for instance) MediatR.


Names are important: data tells me nothing. Ditto jo. Ditto responseMsg2. Ditto response2. Ditto match1.

Or consider var specCaseRequest= unitOfWork.GetSigningCaseRepository();: it doesn't make sense to me that the method GetSigningCaseRepository results in a variable called specCaseRequest.

What is ResponseMessage()? Looks like you use it as a method -- IHttpActionResult response2 = ResponseMessage(responseMsg2); -- yet its name sounds like a class name.


Instead of "", use string.Empty, e.g. in if (specCase.Name == "").


You seem to have copy-pasted this almost a dozen times, even though there are only minor differences between each version:

JObject match = jo.Values<JObject>()
    .Where(m => m["Id"].Value<int>() == 2)
    .FirstOrDefault();
responseMsg2.Headers.Location = new Uri(Url.Link("DefaultApi", new { controller = "SpecCases", id = specCase.Id }));
responseMsg2.Content = new StringContent(match.ToString(), Encoding.UTF8, "application/json");
responseMsg2.Content.ReadAsStringAsync();
IHttpActionResult response2 = ResponseMessage(responseMsg2);
return response2;

Do not copy-paste; instead move such lines to a method with the appropriate parameters.


I find it a bit odd that you make a distinction between if (specCase.Name == "") and if (specCase.Name == null).

Ditto for x.SignatoryId == null and x.SignatoryId == "".

Meanwhile you only check specCaseRequest.Signatories.Select(x => x.Id == "") and not specCaseRequest.Signatories.Select(x => x.Id == null).

\$\endgroup\$
-7
\$\begingroup\$

First off, it is generally considered bad practice to create and use global variables. You could probably combine a few of the elif statements. You could also use case (if that language has it) but that is not too much better.

\$\endgroup\$
  • 5
    \$\begingroup\$ Where are the global variables in this code? It is a single, self-contained method. How would some of the else if statements be combined? Can you give a specific, concrete example? And of course C# has switch/case—if you're going to review code written in a language, you should at least be familiar with the language's supported constructs! But how would you use switch/case here with strings and LINQ? \$\endgroup\$ – Cody Gray Jan 24 '17 at 8:24
  • \$\begingroup\$ Read cheerfully, combine a few of the elif statements meant factor out common code. \$\endgroup\$ – greybeard Jan 24 '17 at 10:37

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.