Philippe Jausions [2005-08-11 23:04 UTC] In my tests sprintf('"%s"', $str) is much slower than '"' . $str . '"'
In reduce_string() use an array of patterns instead of 3 independent calls to preg_replace. and simply "return trim($str);" instead of assigning + return.
Otherwise some CS problems: put spaces before the opening parenthesis after "if" "switch" "for" and the like.
Doing ++$loop instead of $loop++ in "for" loops is a little bit faster.
In decode(), use preg_match('/^("|\').+\1$/s') instead of 2 preg_match(). And farther down fix buggy regex to preg_match('/^\(.*\)$/s')
The ObjectFromJSON class name is not PEAR CS compliant. Any objection to use stdClass instead, although it may be use full to have a specific class for JSON-created objects? You could accept a class name to be used too.
On the same topic, Services_JSON is illsuited IMO. Maybe PHP_JSON? Anyway, whatever name it will be, the constants and class names will need to be corrected.
Michal Migurski [2005-08-12 18:28 UTC] Thanks for the comments. I made the coding standards changes and a few of the preg_* edits. I assume your "buggy regex" referred to the one that matches curly braces, not parentheses, yes? I switched to stdClass as well.
Not sure about the Services_JSON/PHP_JSON naming, but I have no preference either way.
bertrand Gugger [2005-08-12 21:42 UTC] Your class should be named "Services_JSON" or "PHP_JSON" but not "JSON"
Looks like everything is static, $this only for $this->use ? Is it not quite a constant ?
I'm still wondering if such a processing for each char (encode strings) could not be reduced ... no dichotomy possible ?
Anyway, I appreciate your refactoring of this part even if switch(){} is not so popular.
Hope to use it ... and forget I use it.
Bon courage.
Philippe Jausions [2005-08-29 20:40 UTC] Did you update the link with your changes? I still see an awful lot of sprintf("%s")...
For the object type in decode(), it could be useful to specify the class to use to generate the instance.
I am thinking into adding a JSON backend to Services_Webservice (using this package of course...)
Arnaud Limbourg [2005-09-23 09:23 UTC] Hi,
What is needed to move this proposal forward ?
Michal Migurski [2005-10-08 19:00 UTC] I will be factoring out a few sprintf's this week, and changing the class name to match the proposal name.
Justin Patrin [2005-10-08 22:36 UTC] sprintf('%d', $var) and sprintf('%f', $var) should really be (int)$var and (float)$var. Sprintf is a slow solution and it very rarely, if ever, needed.
sprintf('{%s}',and '[%s]', same thing. It makes no sense to do simple string inserting with sprintf. Use '{'.(string).'}' and '['.(string).']'
Why both enc() and encode() and dec() and decode()?
Again, sprintf("%s:%s"), not ok. This is simple concatenation. Use (string).':'.(string).
'/^("|\').+("|\')$/s'
Perhaps you mean:
'/^("|\').+\1$/s'
$c+=1 should be ++$c
Please put each line of code on its own line. Ex:
case '\b': $utf8 .= chr(0x08); $c+=1; break;
Should be:
case '\b':
$utf8 .= chr(0x08);
++$c;
break;
And:
$utf8 .= substr($chrs, $c, 2); $c += 1;
Should be:
$utf8 .= substr($chrs, $c, 2);
++$c;
preg_match('/^\[.*\]$/s', $str) || preg_match('/^\{.*\}$/s', $str)
can be reduced to:
preg_match('/^([\[\{]).*\1$/s', $str)
"\\" should be '\\'
There seems to be something wrong with your indeting (see end of decode()).
Is there a reason the string needs to be reduced before being decoded? It seems that if decode() handles comments (and it seems to) then reduce_string() is basically just repeated code.
Michal Migurski [2005-10-10 05:10 UTC] Thanks Justin - those all sound like good suggestions. I generally prefer sprintf() because it looks cleaner in my editor, but if there's a performance reason to eschew it I'm happy to switch.
dec() and enc() are just shorthand synonyms for the encode() & decode() methods.
Michal Migurski [2005-10-10 05:16 UTC] reduce_string() is called in two places to handle two possible locations for "/*...*/" style comments - once at the very start of decode(), to account for comments at the start & end of the entire JSON string, and again inside the array/object literal parsing area to account for comments inside brackets.
This is the only way I know of to remove such comments without accidentally munging strings that may contain "/*" or "*/".
bertrand Gugger [2005-10-13 12:30 UTC] Just for uncommenting the sources, why not use :
[code]
preg_replace( '#(?:/\*(?:(?R)|.)*\*/|//.*$)|(([\'"])(?:.*)(?<!\\\\)\2|([^\'"/]+))#msU',
"$1", $source);
[/code]
|
