4
\$\begingroup\$

I'm currently using this method to test a string if it valid JSON. The JToken class depends on Newtonsoft.Json.Linq.

public static bool IsValidJson(this string stringValue)
{
    bool returnValue = false;
    string value = null;

    if (!String.IsNullOrEmpty(stringValue))
        value = stringValue.Trim();

    if (!String.IsNullOrWhiteSpace(value) &&
        ((value.StartsWith("{") && value.EndsWith("}")) || //For object
         (value.StartsWith("[") && value.EndsWith("]")))) //For array
    {
        try
        {
            value = stringValue.Trim();

            var obj = JToken.Parse(value);
            returnValue = true;
        }
        catch
        {
            returnValue = false;
        }
    }
    else
    {
        returnValue = false;
    }

    return returnValue;
}

Any recommendations to improve?

\$\endgroup\$
  • 2
    \$\begingroup\$ I wonder why do you need to do this at all? \$\endgroup\$ – t3chb0t Feb 25 '17 at 8:10
4
\$\begingroup\$

You can reduce indenting by checking the string value upfront to check if it's null or whitespace. No need for the check if IsNullOrEmpty as IsNullOrWhiteSpace accounts for that check as well. You are also trimming the stringValue twice and reassigning it to value object.

Also you should only catch exceptions that you can handle.

public static bool IsValidJson(this string stringValue)
{
    if (string.IsNullOrWhiteSpace(stringValue))
    {
        return false;
    }

    var value = stringValue.Trim();

    if ((value.StartsWith("{") && value.EndsWith("}")) || //For object
        (value.StartsWith("[") && value.EndsWith("]"))) //For array
    {
        try
        {
            var obj = JToken.Parse(value);
            return true;
        }
        catch (JsonReaderException)
        {
            return false;
        }
    }

    return false;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ This fails the Unit Test when you provide a value that doesn't contain an opening bracket. You need the else condition to return false. Or have the last statement in the if logic return true, and the last statement return false. \$\endgroup\$ – Josh Feb 25 '17 at 14:39
  • \$\begingroup\$ @josh good Catch \$\endgroup\$ – CharlesNRice Feb 27 '17 at 14:31
1
\$\begingroup\$

JContainer is the base class for:

  • JArray
  • JObject
  • JProperty

Here would be your new method using the JContainer.Parse() method:

public static bool IsValidJson(this string value)
{
    try
    {
        var json = JContainer.Parse(value);
        return true;
    }
    catch
    {
        return false;
    }
}
\$\endgroup\$
  • \$\begingroup\$ My thought was that it is expensive to validate via Exceptions, so I ran the simplest tests first before calling JToken.Parse(). \$\endgroup\$ – Josh Feb 25 '17 at 17:39
  • \$\begingroup\$ @Josh To be more specific, it's not expensive to validate, rather it's expensive to catch. So, if you are passing valid JSON there is no performance problem here. \$\endgroup\$ – Svek Feb 25 '17 at 17:45
  • \$\begingroup\$ correct, only failed validations are we dealing with the cost of exceptions. \$\endgroup\$ – Josh Feb 25 '17 at 17:48
  • \$\begingroup\$ When I tried replacing JToken with JContainer, ReSharper gave me a Name can be simplified suggestion, which was to change JContainer to JToken. This is because JContainer.Parse is using JToken.Parse. \$\endgroup\$ – Josh Feb 25 '17 at 17:53
1
\$\begingroup\$

Well, no code samples, just some general considerations here.

To give you any advice, first of all we need to know, what is your goal? What is the mission of this code?

In one of the comments you mention something about cost. If it is valid to assume that you need to process many such JSONs concurrently, I'd optimise for throughput and memory and use a streaming parser. This type of parser is effectively a state-machine, which takes symbol by symbol and makes a transition to the next state according to its previous state and processed symbol. This is basically the same method which is used in processing regular expressions.

If on some step you find out that no valid transition can be made - throw a (single) exception.

As an output from this parser you then can produce a stream of more high-level values than just JSON tokens. Say, if you need to process large array, you can push to that stream every element of this array. I think, you could pass JSON-Path expression as an input to flush to stream every time when matching JSON structure ends.

In addition to being quite effective, this approach would give you clean, readable and testable code which is aligned with JSON semantics. Though state-machines can be quite verbose.

If you'd like to accomplish it as an exercise - this would be good approach. But if you just need some working code - use an existing JSON streaming parser.

\$\endgroup\$
  • 1
    \$\begingroup\$ I find this is the best answer. The only one questioning the current approach that seems to be wastefull and useless but unfortunatelly OP didn't care to provide more feedback. \$\endgroup\$ – t3chb0t Feb 27 '17 at 15:29
1
\$\begingroup\$

And yet another version. A simple flip of the initial test ..... @ t3chb0t, as I can NOT comment ... Hum, This version is more concise covers ALL cases with the desired return value. How do you think it is inferior AND/OR a simple code dump with no thought given to it?

public static bool IsValidJson(string stringValue)
{
  if (string.IsNullOrWhiteSpace(stringValue) == false)
  {
    var value = stringValue.Trim();
    if ((value.StartsWith("{") && value.EndsWith("}")) || //For object
        (value.StartsWith("[") && value.EndsWith("]"))) //For array
    {
      try
      {
        var obj = JToken.Parse(value);
        return true;
      }
      catch (JsonReaderException)
      {
        return false;
      }
    }
  }

  return false;
}
\$\endgroup\$
  • \$\begingroup\$ Code dumps are off-topic. Please provide some explanation, why is this better? Why should this version be used? \$\endgroup\$ – t3chb0t Feb 27 '17 at 15:00
  • \$\begingroup\$ I didn't say it's inferior but a code dump because you didn't explain what you did, only posted the code. On the other hand wrapping everything with an if is not a great improvement, on the contrary, often considered a bad practice. \$\endgroup\$ – t3chb0t Feb 27 '17 at 15:20
  • \$\begingroup\$ @t3chb0t - There was an explanation that all I did (vs how I would have done it) flip the conditional to achieve ALL paths giving an intended answer. If you think that fragmented code, as in early exits and undefined return values is good vs code that is designed to cover all cases then our real life experiences are ... very different. \$\endgroup\$ – Enigma Maitreya Feb 27 '17 at 15:57
0
\$\begingroup\$

Do you need a separate value for the input?
Do you need to have separate test for null or white space ?
Just trim it and run the one condition - if it is empty or white space it will quickly fail that test.
Also from Charles only the exception you can handle.

public static bool IsValidJson(this string stringValue)
{
    stringValue = stringValue.Trim();   
    if ( (stringValue.StartsWith("{") && stringValue.EndsWith("}")) //For object  
         ||
         (stringValue.StartsWith("[") && stringValue.EndsWith("]")) //For array
       )  
    {
        try
        {
            var obj = JToken.Parse(stringValue);
            return true;
        }
        catch (JsonReaderException)
        {
            return false;
        }
    }
    else 
    {
        return false;
    }
}  
\$\endgroup\$

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.