Testing whether a string contains valid JSON by calling json_decode()

Question

I had a code review and got told by the senior programmer that it was very bad programming practice to use the json_decode() function of php to determine if a string was not in JSON format.

My argument was that the json_decode function has a fail case (returning NULL) and this exists for reasons such as the purpose I was using it for.

I got sat down one on one and told to never pass data to a function that was not intended for the function (in this case referring to passing a hex string to the json_decode() function).

Is what I did bad coding practice? How should I have written the code?

Here is the php code of what I basically did:

//example JSON Data
/*
$ExampleData = '
{
    "test": "48656c6c6f20576f726c64"
}
';
*/

//the most common case is that a hex string sent to the php script
$ExampleData = "48656c6c6f20576f726c64";

$jsonData = json_decode($ExampleData);

//one in every 1000 responses is JSON data
if($jsonData)
{
    //extract varaibles from the json
    print ("JSON data recieved");
}
else
{
    //response is hex so carry out normal function
    print ("Hex Data recieved");
}

try it online

Forgot to add, the suggestion by the senior developer was to use json_decode and instead read the first character of the string and if it is a "{" then run json_decode on the "correctly" formatted string. Does this type of solution have a name? why is it better than what I did? — AKMorris
– AKMorris, Commented Aug 9, 2017 at 5:29
this is awfully bad practice, so is the example code given by Sith Lord Common. if you don't already know whether or not your variable is a json data or not you have much bigger problems. there should never be that level of ambiguity in well written code. you should have responded to the senior programmer by telling him the code sucked to begin with. — I wrestled a bear once.
– I wrestled a bear once., Commented Aug 9, 2017 at 17:49
I see, thank you for the constructive criticism. The funny thing is it is the senior developer's code that is supplying me with the hex/json string. I will have to request he add some sort of header and pass an object to me instead. — AKMorris
– AKMorris, Commented Aug 9, 2017 at 23:06

Mike Brant · Accepted Answer · 2017-08-10 14:22:13Z

4

I strongly agree with comment by @Iwrestledabearonce. When dealing with input, there are normally other means to understand content type (content type headers, additional input fields, or other means to express an explicit contract). Having forked code paths like this probably means you have a bad design and you should make sure the contract with data source is clear around how to convey what type of data is being passed. That doesn't mean that the data passed still doesn't need to be validated as hex or JSON input, just that you shouldn't have to rely on performing JSON decoding or regex matching to determine your input data type.

This is especially exacerbated by the fact you mention 1 in 1000 request may have JSON. Do you really want to perform json_decode() step on all 999 other requests?

edited Aug 10, 2017 at 14:22

answered Aug 9, 2017 at 19:32

Mike Brant

9,87814 silver badges24 bronze badges

\$\begingroup\$ So basically you are saying that who ever wrote the code that supplies the hex/json string to my php should instead pass something like a Json object that has a header field (specifying hex or json) and a data field (containing the actual hex or json)? That way I just read the header to determine the datatype? I like that. \$\endgroup\$

AKMorris
– AKMorris

2017-08-09 23:01:25 +00:00
Commented Aug 9, 2017 at 23:01
1

\$\begingroup\$ I think best will be to have two classes, "Hex" and "Json", both can have only the value and implemented __toString(). Only to check if the sting starts with '{' is not good as well. The type should be really known when passed and not to guess.. \$\endgroup\$

martin.malek
– martin.malek

2017-08-10 04:44:08 +00:00
Commented Aug 10, 2017 at 4:44
\$\begingroup\$ @AKMorris if you can control the service, why on the earth you cannot just make it to return consistent results? \$\endgroup\$

Your Common Sense
– Your Common Sense

2017-08-10 04:46:42 +00:00
Commented Aug 10, 2017 at 4:46
\$\begingroup\$ @martin.malek interesting idea to create classes but I am thinking it is a bit overkill for what I need. At the moment I am trying to get my coworker to send the data as an object with packet.header="JSON (or) HEX" and packet.data="hex or json string here". Then I just check if packet.header is "JSON" or "HEX". However if I can argue the need for creating custom json and hex object I will... I just need a reason why it is better than the packet.header option... \$\endgroup\$

AKMorris
– AKMorris

2017-08-10 04:56:42 +00:00
Commented Aug 10, 2017 at 4:56
\$\begingroup\$ @Your Common Sense if I could do that there would be no need for my opening question in the first place so what you are asking is irrelevant. \$\endgroup\$

AKMorris
– AKMorris

2017-08-10 04:58:42 +00:00
Commented Aug 10, 2017 at 4:58

| Show 1 more comment

Your Common Sense · Accepted Answer · 2017-08-09 08:35:11Z

3

Well, there could be a problem.

Imagine you've got JSON, but a malformed one? Your code would count it as a HEX value (why a service would return such inconsistent results is another question). So indeed it's better to validate the hex string first and only process JSON otherwise.

//example JSON Data
/*
$ExampleData = '
{
    "test": "48656c6c6f20576f726c64"
}
';
*/

//the most common case is that a hex string sent to the php script
$ExampleData = "48656c6c6f20576f726c64";

if (preg_match('~^[0-9a-fA-F]+$~',$ExampleData))
{
    //response is hex so carry out normal function
    print ("Hex Data recieved");

} else {

    $jsonData = json_decode($ExampleData);
    if(json_last_error())
    {
        //extract varaibles from the json
        print ("JSON data recieved");
    } else {
        // JSON error
        throw new \Exception(json_last_error_msg());
    }
}

Note that I used json_last_error() function because (in general) the legitimate decoded result could be 0 or FALSE which will trigger an error for no reason.

edited Aug 9, 2017 at 8:35

answered Aug 9, 2017 at 8:26

Your Common Sense

9,1431 gold badge22 silver badges51 bronze badges

\$\begingroup\$ To play devils advocate: In your case imagine you've got Hex, but it is malformed? For example one character is missing off the end? Then do we need to make sure we are getting hex pairs as well? \$\endgroup\$

AKMorris
– AKMorris

2017-08-09 08:33:45 +00:00
Commented Aug 9, 2017 at 8:33
\$\begingroup\$ It is up to you. Given both formats are supposed to return the same hex string, I find it convenient to validate it after decoding. \$\endgroup\$

Your Common Sense
– Your Common Sense

2017-08-09 08:38:15 +00:00
Commented Aug 9, 2017 at 8:38
\$\begingroup\$ Well as I see it what occurs in the if statement is irrelevant to the question. I can still validate the Json in my original code the same way you have done. In regards to your answer, the real question is which of these is better to use and why? Is it better to use "if (json_decode($ExampleData))" or "if (preg_match('~^[0-9a-fA-F]+$~',$ExampleData)" \$\endgroup\$

AKMorris
– AKMorris

2017-08-09 08:42:22 +00:00
Commented Aug 9, 2017 at 8:42
\$\begingroup\$ The answer is above. validating for hex makes you able to spot a malformed json. There are at least TWO possible reasons for json_decode to fail: a legitimate hex value and an error. In your code you are expecting only one and thus fail to detect the error. With my code you are handling both properly. Do you understand it now? \$\endgroup\$

Your Common Sense
– Your Common Sense

2017-08-09 08:46:35 +00:00
Commented Aug 9, 2017 at 8:46
\$\begingroup\$ So you believe what I did is bad coding practice because the Hex or Json could be malformed? Couldn't that check be done after the fact (after the if statement has determine hex or json)? \$\endgroup\$

AKMorris
– AKMorris

2017-08-09 08:54:38 +00:00
Commented Aug 9, 2017 at 8:54

| Show 3 more comments

AKMorris · Accepted Answer · 2017-08-11 02:17:18Z

-1

As I can only receive a string to this php file I ended up ended up using a packet.header decoded in to a php object from a string like so:

<?php
//example JSON Data
/*
$packet->header = "JSON";
$packet->data = '
{
    "test": "48656c6c6f20576f726c64"
}
';
*/

//the most common case is that a hex string sent to the php script
$packet->header = "HEX";
$packet->data = "48656c6c6f20576f726c64";

if ($packet->header == "HEX")
{
    //do validation and process data
    print ("HEX data recieved");
}
elseif ($packet->header == "JSON")
{
    //do validation and process data
    print ("JSON data recieved");
}
else
{
    //data is not expected data or data is corrupt
}


?>

What does everyone think of this solution?

answered Aug 11, 2017 at 2:17

AKMorris

1155 bronze badges

1

\$\begingroup\$ Nothing bad with this solution, save it's irrelevant to the question asked. \$\endgroup\$

Your Common Sense
– Your Common Sense

2017-08-11 05:01:31 +00:00
Commented Aug 11, 2017 at 5:01
\$\begingroup\$ I agree with Sith Lord Common. This is a much better solution, but you should have posted this as a new question rather than an answer to your own question. \$\endgroup\$

I wrestled a bear once.
– I wrestled a bear once.

2017-08-11 13:10:23 +00:00
Commented Aug 11, 2017 at 13:10

Add a comment |

Stack Exchange Network

Testing whether a string contains valid JSON by calling json_decode()

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Testing whether a string contains valid JSON by calling json_decode()

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions