Extracting command data from a REST API

Question

I have a method in C# which communicates with REST API.It has a several small task like splitting of a string ,checking a commandText is valid or not,POST and GET JSON payload to/from REST API.

  public  string PostToAPI([FromBody]string value)
        {
            string input = value;
            //splitting of string into substrings
               var str = input.Split(new[] { ' ' }, 2);
                var payload = new testModel
               {
                   Command = str[0],
                   CommandText = str[1]
               };
            //list of commands
              List<string> cmdText = new List<string>(){"/calc", "/test" };
            //check CommandText is valid or not
              if(cmdText.Contains(payload.CommandText))
              {
               //return valid
              }

            // Serialize our concrete class into a JSON String
            var stringPayload = JsonConvert.SerializeObject(payload);
             try
             {
                 // Wrap our JSON inside a StringContent which then can be used by the HttpClient class
                 var httpContent = new StringContent(stringPayload, Encoding.UTF8, "application/json");

                  using (var httpClient = new HttpClient())

                 {
                     HttpResponseMessage message = httpClient.PostAsync("https://testapi.com", httpContent).Result;
                     if (message.IsSuccessStatusCode)
                     {
                         string result = message.Content.ReadAsStringAsync().Result;
                         return result;
                     }
                     else
                     return message.StatusCode+"kindly contact to administrator";

                 }
             }
             catch (Exception ex)
             {
                 return ex.InnerException.ToString();
             }

            }

splitting of a string splits the string into two substrings.

checking a commandtext, checks commandtext is valid or not.

they all are in one method.From architecture/design point of view is this right or should i split the task into different module or in different classes to add more flexibility, readability,As there may be a situation where i just need to add commands string and don't want to touch the other task.

How to organize such c# tasks into different modules/class to make them more flexible, readable? Thanks!

In the title you should describe what your code does and not what you want to do with it. — t3chb0t
– t3chb0t, Commented Aug 17, 2016 at 6:30
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. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. — BCdotWEB
– BCdotWEB, Commented Aug 17, 2016 at 7:51

BCdotWEB · Accepted Answer · 2016-08-17 08:56:27Z

What's the point of string input = value;? Why not work with value directly?

Comments should tell me why, not what. If you suspect I cannot detect //splitting of string into substrings from your code, then your code is bad.

testModel is a class and thus should be PascalCase;.

cmdText is a collection of commands? That variable name doesn't suggest so at all. No wonder you need to preface it with //list of commands. If it is a list of commands, call it commands. Or considering the context, validCommands.

What's the point of this:

if(cmdText.Contains(payload.CommandText))
{
    //return valid
}

This doesn't do anything.

You start out using var, yet later on you get more explicit: HttpResponseMessage message, string result. Be consistent.

Same for your if...else:

if (message.IsSuccessStatusCode)
{
    string result = message.Content.ReadAsStringAsync().Result;
    return message.Content.ReadAsStringAsync().Result;
}
else
return message.StatusCode+"kindly contact to administrator";

The bad indentation hide that return message.StatusCode+"kindly contact to administrator"; is actually your else. Always use brackets!

What's the point of string result = message.Content.ReadAsStringAsync().Result;? Why not simply return message.Content.ReadAsStringAsync().Result;?

Quite frankly this code looks very unfinished. Your style is inconsistent, indentation is all over the place, there are superfluous blank lines (after using (var httpClient = new HttpClient()) for instance), the comments are superfluous, there's code in there that doesn't do anything,...

This also looks like a method in a Controller, and that's worrying. Because even if it's a mere 50 lines, if you have several methods like this pretty soon your controller class will be very bloated. Moreover this one method does three things:

convert the incoming data into a payload
checking the payload against a list of values
posting the payload to another API

Each of these should be their own method, and thus the body of this controller should be its own class, split into multiple methods. Look for instance at MediatR to "un-bloat" your controllers.

Stack Exchange Network

Extracting command data from a REST API

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Extracting command data from a REST API

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions