Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I've created a set of classes thatwork together to create the html needed to display social plugins like Facebook like, google plus and twitter. At firsti defined an abstract class SocialButtonAbstract that is the base class that will be extended by the various plugins

abstract class SocialButtonAbstract {
    protected $settings;
    function __construct(array $settings = null) {
        $this->_mergeSettings($settings);
    }

    protected function _mergeSettings(array $settings){
        if ($settings !== null){
            $this->settings = array_merge($this->settings, $settings);
        }
    }

    abstract public function renderButton(array $settings = NULL);
    abstract public function renderScript();
}

This is an example of FacebookLike (i post this example because it's central to my question)

require_once 'SocialButtonAbstract.php';
Class FacebookLike extends SocialButtonAbstract{
    const FB_SEND = 'fb:send';
    const FB_HREF = 'fb:href';
    const FB_WIDTH = 'fb:width';
    const FB_LAYOUT = 'fb:layout';
    const FB_SHOW_FACES = 'fb:show-faces';
    const FB_ACTION = 'fb:action';
    const FB_FONT = 'fb:font';
    const FB_COLORSCHEME = 'fb:colorscheme';
    const FB_REF = 'fb:ref';

    private $_apyKey = null;


    protected $settings = array(
            self::FB_SEND => 'true',//Display button SEND
            self::FB_HREF => FALSE,//the URL to like. If FALSE defaults to the current page.
            self::FB_WIDTH => '88',// the width of the Like button.
            self::FB_LAYOUT => "button_count", //there are three options standard, button_count and box_count
            self::FB_SHOW_FACES => 'false', //specifies whether to display profile photos below the button (standard layout only)
            self::FB_ACTION => 'like',//the verb to display on the button. Options: 'like', 'recommend',
            self::FB_FONT => FALSE,//the font to display in the button. Options: 'arial', 'lucida grande', 'segoe ui', 'tahoma', 'trebuchet ms', 'verdana'
            self::FB_COLORSCHEME => 'light',// the color scheme for the like button. Options: 'light', 'dark'
            self::FB_REF => FALSE// I don't understend how this works, for now is a Todo
    );

    public function setApiKey($apyKey){
        $this->_apyKey = $apyKey; 
    }


    public function renderScript(){
        $apiKey = $this->_apyKey;
        if ($apiKey !== null){
            $apiKey = "&appId=$apiKey";
        }else{
            $apiKey = '';
        }
        $markup = <<<HTML
<div id="fb-root"></div>
<script>
(function(d, s, id) {
        var js, fjs = d.getElementsByTagName(s)[0];
        if (d.getElementById(id)) return;
        js = d.createElement(s); js.id = id;
        js.src = "//connect.facebook.net/en_US/all.js#xfbml=1$apiKey";
        fjs.parentNode.insertBefore(js, fjs);
}(document, 'script', 'facebook-jssdk'));
</script>

HTML;
        return $markup;
    }

    public function renderButton(array $settings = null){
        $this->_mergeSettings($settings);
        $dataAttributes = array();
        foreach ($this->settings as $attr => $value){
            //Skip the values thatare FALSE, no need to render anything for them
            if($value === FALSE){
                continue;
            }
            //convert the keys from the format fb:send to the HTML5 data-send
            $attr = str_replace("fb:", "data-", $attr);
            $dataAttributes[$attr] = $value;
        }

        $markup = "<div class=\"fb-like\" ";
        foreach ($dataAttributes as $dataAttr => $dataValue){
            $markup .= "$dataAttr=\"$dataValue\" ";
        }
        $markup .= "></div>";
        return $markup;
    }

}

To display the various plugins i created a little helper class that provides the markup for the elements that surround the social buttons

class SocialPluginsHelper {

    const ID_OF_WRAPPER = 'id_of_wrapper';
    const CLASS_OF_WRAPPERS = 'class_of_wrappers';
    const HTML_BEFORE_BUTTONS = 'html_before_buttons';
    const HTML_AFTER_BUTTONS = 'html_after_buttons';
    const CUSTOM_PLUGIN_STYLE = 'custom_plugin_style';
    const TAG_OF_WRAPPER = 'tag_of_wrapper';
    const STYLE_OF_WRAPPER = 'style_of_wrapper';

    function __construct(array $settings = null) {
        if ($settings !== null){
            $this->settings = array_merge($this->settings, $settings);
        }
    }

    private function createStyleAttribute($style){
        $customStyle = '';
        if(!empty($style)){
            $customStyle = "style=\"$style\"";
        }
        return $customStyle;
    }

    private $plugins = array();
    private $settings = array(
            self::ID_OF_WRAPPER => "socialButtons",//id of the wrapper element
            self::TAG_OF_WRAPPER => "div",//what HTML element to use as a wrapper, either DIV or UL (if you choose DIV, alle the children will be DIV too otherwise the will be LI)
            self::CLASS_OF_WRAPPERS => "social",//Class of the children element
            self::HTML_BEFORE_BUTTONS => '',//custom HTML before the plugins
            self::HTML_AFTER_BUTTONS => '',//custom HTML after the plugins
            self::STYLE_OF_WRAPPER => '',//custom style of the main wrapping element
            self::CUSTOM_PLUGIN_STYLE => array()//this is some custom style thatwill be added to the elements that surround the plugins, useful to add extra width or inline options. It's                                        //an associative array  with the keys that equals the name of the classes (for example to add style to the Facebook plugin, use the key FacebookLike
    );

    public function add(SocialButtonAbstract $plugin){
        $this->plugins [] = $plugin;
    }

    public function renderAllButtons(array $pluginSettings = null){
        $settings = $this->settings;
        $tagOfChildren = $settings[self::TAG_OF_WRAPPER] === "div" ? "div" : "li";
        $tagOfWrapper = $settings[self::TAG_OF_WRAPPER];
        $idOfWrapper = $settings[self::ID_OF_WRAPPER];
        $htmlBeforeButtons = $settings[self::HTML_BEFORE_BUTTONS];
        $classOfWrappers = $settings[self::CLASS_OF_WRAPPERS];
        $htmlAfterButtons = $settings[self::HTML_AFTER_BUTTONS];
        $customStyleOfWrapper = $this->createStyleAttribute($settings[self::STYLE_OF_WRAPPER]);
        $markup = "<$tagOfWrapper $customStyleOfWrapper id=\"$idOfWrapper\">\n";
        $markup .= "$htmlBeforeButtons\n";
        foreach($this->plugins as $plugin){
            $customSettings = isset($pluginSettings[get_class($plugin)]) && is_array($pluginSettings[get_class($plugin)]) ? $pluginSettings[get_class($plugin)] : array();
            $customStyle = isset($settings[self::CUSTOM_PLUGIN_STYLE][get_class($plugin)]) ? $this->createStyleAttribute($settings[self::CUSTOM_PLUGIN_STYLE][get_class($plugin)]) : '';
            $markup .= "<$tagOfChildren class=\"$classOfWrappers\" $customStyle>".$plugin->renderButton($customSettings)."</$tagOfChildren>\n";
        }
        $markup .= "$htmlAfterButtons\n</$tagOfWrapper>\n";
        echo $markup;
    }

    public function renderAllScripts(){
        foreach($this->plugins as $plugin){
            echo $plugin->renderScript();
        }
    }
}

I also create a factory class to create instances of the plugins but it's not relevant to my question. I use the plugin like this:

    //Initialize the helper with some custom markup options
$socialPlugins = new SocialPluginsHelper(array(
        SocialPluginsHelper::HTML_AFTER_BUTTONS => '<div class="clear"></div>',
        SocialPluginsHelper::HTML_BEFORE_BUTTONS => '<div class="clear"></div>',
        SocialPluginsHelper::STYLE_OF_WRAPPER => 'margin-top:20px;',
        SocialPluginsHelper::CUSTOM_PLUGIN_STYLE => array(
                "TwitterButton" => "width:69px;",
                "FacebookLike" => "width:88px;")
)
);
    //Add the facebook plugin with some custom option
$socialPlugins->add(PluginFactory::create("FacebookLike", array(FacebookLike::FB_SEND => "false")));
    //Add the Twitter plugin with no custom option
$socialPlugins->add(PluginFactory::create("TwitterButton"));
    //Add the Gplus plugin with no custom option
$socialPlugins->add(PluginFactory::create("Gplus"));

Now here comes the part i'm less convinced of, if i have multiple Facebook Like button on the page i need to pass in the href to like for each plugin. for this reason, i do something like

$socialPlugins->renderAllButtons(array("FacebookLike" => array(FacebookLike::FB_HREF => $url)));

I used as a convention that i pass an array that has the name of the classes as "keys" amd an array of $settings as values. Whati'm askin is

  • in general, do you think that my class design is correct?
  • is there some design pattern i should have implemented?
  • am i doing the right thing by overriding the $settings each time or should i create a new instance of the plugin every time?

Thanks for your time

share|improve this question
Is the question too long?Should i take out the code? – Nicola Peluchetti Mar 19 '12 at 20:27
1  
I think this is just good code. Although I'd fix a few HTML injection problems. – usr Mar 30 '12 at 22:19
@usr where do you think that the HTML injection problems lie? – Nicola Peluchetti Mar 31 '12 at 1:30
""$dataAttr=\"$dataValue\" "" Just like that. I don't know PHP but this looks unencoded to me. – usr Mar 31 '12 at 10:35

1 Answer

up vote 1 down vote

I know this is rather old, but I came across it while look through the unanswered pile and thought there were a few things I could add. Because it is so old and I'm unsure if you are still watching this, I won't go into a lot of detail. However, if you want me to, I will of course return and do a more thorough job. That being said...

Type Hinting

Type hinting forces your methods to a strict set of rules. When you say your argument is going to be an array, it MUST be an array. The only time this is "ignored" is when you give it a default value, such as NULL. Should you ever pass that argument, with its default value, to another method that does not expect the default value, it will produce errors. Your _mergeSettings() method is such. It gets its argument from the methods that call it, which have those default NULL values, but _mergeSettings() does not expect a NULL default. So of course, should you ever neglect to pass anything to those parent methods, _mergeSettings() will throw so many errors your head will spin.

Ternary Operators

Maybe you know about them. Not everyone likes them. However, I think your renderScript() method has a good example of where one would be acceptable. If you're not sure what they are google them, and try not to abuse them. Well abuse them so you learn how to use them, then go back and undo it all. Most people when they first learn about ternary operators use them too much and their code becomes illegible as a consequence. Just know that they are there to help not hinder, so use them only when they won't hinder your code, this means legibility too. Or just don't use them at all, many don't.

public function renderScript(){
    $apiKey = $this->_apyKey !== null ? '&appId=' . $this->_apyKey : '';

HTML Output From Methods

There are a lot of ways to go about generating HTML with PHP. Most people don't like to mix them however. As usr pointed out, it looks odd, and can confuse some people, especially those unfamiliar with PHP. I think its much cleaner to include() HTML when possible, or, in the case that usr is speaking of, escape to PHP from HTML rather than from PHP to HTML. So in the renderButton() method, I would return the $dataAttributes array and use that in the HTML file where you call this class and then escape into PHP when needed.

<div class="fb-like" <?php foreach( $dataAttributes AS $dataAttr => $dataValue ) : echo "$dataAttr=\"$dataValue\" "; endforeach; ?>></div>

I used : and endforeach; in the above example instead of {} because this is how this is usually done in HTML documents. However, in this instance, it probably would be better to use the latter. Whichever improves legibility, I used the other simply to show you.

In your renderScript() method I would just use an include and save that HTML to a separate file. Or at the very least, drop the variable and just return the heredoc. The $markup variable is otherwise unused, so unnecessary.

    return <<<HTML
<div id="fb-root"></div>
<script>
(function(d, s, id) {
    var js, fjs = d.getElementsByTagName(s)[0];
    if (d.getElementById(id)) return;
    js = d.createElement(s); js.id = id;
    js.src = "//connect.facebook.net/en_US/all.js#xfbml=1$apiKey";
    fjs.parentNode.insertBefore(js, fjs);
}(document, 'script', 'facebook-jssdk'));
</script>

HTML;
share|improve this answer
Thanks for reviewing this :) Your help is really appreciated, it's something i wrote some time ago and it was my first real "object oriented" approach to code, i agree on all the things you point out. Personally i don't like includes because it means "one more file to open" but i agree with you that they would help in this case. – Nicola Peluchetti Jul 3 '12 at 22:29

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.