Skip to main content
Code Review

Return to Answer

added 1619 characters in body
Source Link
Mike Brant
Mike Brant
  • 9.9k
  • 14
  • 24

Why not have Wordpress code transform the post to HTML for you as it already does for your regular site? One of the unfortunate parts of the Wordpress architecture is that it does not cleanly decouple your data from your display concerns (as you are finding out). Since the Wordpress app "owns" translating the posts in the DB into HTML, why would't Wordpress still own this for your mobile view (perhaps vaivia different template (even, even one that just contains HTML fragments for asynchronous delivery to mobile app). This will likely lead to less breakage as changes in your Wordpress site are made, as you would rely on the same view-generation logic.

Now, with regards to your code as is:

  • Consider foreach($post_content_array as $word) instead of for(...) for this main loop. I don't see where you are doing any operations here which require you to refer to the specific index value for the string being evaluated.
  • It seems odd to me that you would pass in a string to this function, yet return an array. I would think this function should return the reassembled string such that the caller doesn't have to understand the inner workings of the function.
  • Consider inverting your conditionals and/or adding continue to de-nest large sections of your code.

For example:

if(strpos($post_content_array[$x], '[caption') !== false){
    $caption_tag_element = true;
    continue;
}
if($caption_tag_element === false){
    array_push($post_content_filtered, $post_content_array[$x]);
    continue;
}
// rest of code, now without nesting

These sorts of "quick exits" can make your code much easier to read and less prone to bugs due to fewer codepaths. There are rarely reasons to have an else condition (like when one of two side-effects need to occur), so try to design them away when possible.

  • You should default to using strict logical comparisons as opposed to their loose counterparts. Very seldom in code do you need the flexibility to evaluate a condition in a type-inspecific manner. Try to use exact comparisons by default to make your code less error prone against unexpected truthy/falsey behaviors.
  • You have WAY too many comments. Let your code speak for itself.

Why not have Wordpress code transform the post to HTML for you as it already does for your regular site? One of the unfortunate parts of the Wordpress architecture is that it does not cleanly decouple your data from your display concerns (as you are finding out). Since the Wordpress app "owns" translating the posts in the DB into HTML, why would't Wordpress still own this for your mobile view (perhaps vai different template (even one that just contains HTML fragments for asynchronous delivery to mobile app). This will likely lead to less breakage as changes in your Wordpress site are made, as you would rely on the same view-generation logic.

Why not have Wordpress code transform the post to HTML for you as it already does for your regular site? One of the unfortunate parts of the Wordpress architecture is that it does not cleanly decouple your data from your display concerns (as you are finding out). Since the Wordpress app "owns" translating the posts in the DB into HTML, why would't Wordpress still own this for your mobile view (perhaps via different template, even one that just contains HTML fragments for asynchronous delivery to mobile app). This will likely lead to less breakage as changes in your Wordpress site are made, as you would rely on the same view-generation logic.

Now, with regards to your code as is:

  • Consider foreach($post_content_array as $word) instead of for(...) for this main loop. I don't see where you are doing any operations here which require you to refer to the specific index value for the string being evaluated.
  • It seems odd to me that you would pass in a string to this function, yet return an array. I would think this function should return the reassembled string such that the caller doesn't have to understand the inner workings of the function.
  • Consider inverting your conditionals and/or adding continue to de-nest large sections of your code.

For example:

if(strpos($post_content_array[$x], '[caption') !== false){
    $caption_tag_element = true;
    continue;
}
if($caption_tag_element === false){
    array_push($post_content_filtered, $post_content_array[$x]);
    continue;
}
// rest of code, now without nesting

These sorts of "quick exits" can make your code much easier to read and less prone to bugs due to fewer codepaths. There are rarely reasons to have an else condition (like when one of two side-effects need to occur), so try to design them away when possible.

  • You should default to using strict logical comparisons as opposed to their loose counterparts. Very seldom in code do you need the flexibility to evaluate a condition in a type-inspecific manner. Try to use exact comparisons by default to make your code less error prone against unexpected truthy/falsey behaviors.
  • You have WAY too many comments. Let your code speak for itself.
Source Link
Mike Brant
Mike Brant
  • 9.9k
  • 14
  • 24

This seems an odd design choice considering you are working in a domain-specific tagging language.

Why not have Wordpress code transform the post to HTML for you as it already does for your regular site? One of the unfortunate parts of the Wordpress architecture is that it does not cleanly decouple your data from your display concerns (as you are finding out). Since the Wordpress app "owns" translating the posts in the DB into HTML, why would't Wordpress still own this for your mobile view (perhaps vai different template (even one that just contains HTML fragments for asynchronous delivery to mobile app). This will likely lead to less breakage as changes in your Wordpress site are made, as you would rely on the same view-generation logic.

If you are truly trying to decouple your mobile app into a service, than perhaps you can work with the cached HTML that is generated by Wordpress vs. the actual database post entry? At least in this manner, you could work with DOM manipulation tools effectively to transform the HTML document (which is really your main problem here as your "caption" is not formatted as HTML).

Even if you decide to forego these alternate approaches, I would say that your could go with a more optimized regex approach assuming that:

  • The [caption ...] and [/caption] items in the posts are truly regularly formed and reliable for use with regex.
  • Your caption text is located before the end tag predictably.

In such a case I would go directly to a replacement using a regular expression. Perhaps something like:

$regex = '#\[caption.*\].*(<img.*/>)(.*)\[/caption\]#mU';
$replace_function = function($matches) {
    $template = <<<'EOT'
<div class="img_wrapper">
    %s
    <span class="photo_caption_text">%s</span>
</div>
'EOT';
    return sprintf($template, trim($matches[1]), trim($matches[2]);
};
$replaced_content = preg_replace_callback($regex, $replace_function, $post_content);

This eliminates should simplify things greatly as opposed to your approach of using regex to break apart the string and then iterating over every single string, evaluating it.