Better approach to extending a class?

[NotionCommotion]

I have a bunch of classes such as BarCharts, PieCharts, etc which extend Charts.  While I didn't show them, all of these classes have other methods as well.  The client sends a request to update a parameter in them.  Originally, I was using a switch statement to ensure the parameter in question was allowed, but found some types of charts allow for different parameters.  I don't wish to duplicate the majority of script which is common to all chart types, and came up with the following, but don't really like it (and obviously don't like my use of cryptic method names such as __updateType3).  Another option is to create a bunch of new classes such as BarChartsUpdate, PieChartsUpdate along with a parent class ChartsUpdate, but this doesn't seem right either.  What would be a better way to do this?  Thank you

$charts=new BarCharts();
$charts->updateParam($_POST['value'],$_POST['attrName']);

namespace MyApp;
class BarCharts extends Charts
{
    protected function _updateXAxis($value)
    {
        return $this->__updateType3($value,'xAxis');
    }
    protected function _updateYAxis($value)
    {
        return $this->__updateType3($value,'yAxis');
    }
}

namespace MyApp;
class PieCharts extends Charts
{
    //...
}

namespace MyApp;
abstract class Charts
{
    public function updateParam($value,$name)
    {
        $method='_update'.ucfirst($name);
        if(method_exists($this,$method)) {
            $error=$this->{$method}($value);
        }
        else {
            $error=$error=\MyApp\ErrorResponse::invalidMethod($name);
        }
        return empty($error)?["status"=>"success"]:$error;
    }
    protected function _updateName($value)
    {
        return $this->__updateType1($value,'name');
    }
    protected function _updateTitle($value)
    {
        return $this->__updateType2($value,'title');
    }
    protected function _updateSubtitle($value)
    {
        return $this->__updateType2($value,'subtitle');
    }

    protected function __updateType1($value,$name)
    {
        if(empty($value)) {
            $error=\MyApp\ErrorResponse::blankValue([$name]);
        }
        else {
            try {
                $stmt = $this->db->prepare("UPDATE charts SET $name=? WHERE id=? AND accounts_id=?");
                $stmt->execute([$value,$this->id,$this->account->id]);
            }
            catch(\PDOException $e){
                $error=\MyApp\ErrorResponse::duplicatedName();
            }
        }
        return isset($error)?$error:null;
    }
    protected function __updateType2($value,$name)
    {
        if(!$this->setParam($name, $value)) {
            $error=\MyApp\ErrorResponse::unknown();
        }
        return isset($error)?$error:null;
    }
    protected function __updateType3($value,$name)
    {
        if(!$this->setSubParam($name, $value)) {
            $error=\MyApp\ErrorResponse::unknown();
        }
        return isset($error)?$error:null;
    }
}

Edited August 29, 2016 by NotionCommotion

[NotionCommotion]

Maybe a better approach?

$charts=new BarCharts();
$charts->updateParam($_POST['value'],$_POST['attrName']);

namespace MyApp;
class BarCharts extends Charts
{
    protected function getMethod($name)
{
$allowed=[
'name'=>'updateType1',
'title'=>'updateType2',
'subtitle'=>'updateType2',
'xAxis'=>'updateType3,
'yAxis'=>'updateType3'
];
return isset($allowed[$name'])?$allowed[$name']:false;
}
}

namespace MyApp;
abstract class Charts
{
    abstract public function getMethod($name);


    public function updateParam($value,$name)
    {
        if($method=$this->getMethod($name) {
            $this->{$method}($value,$name);
        }
        else {
            \MyApp\ErrorResponse::invalidMethod($name);
        }
    }


    protected function __updateType1($value,$name)
    {
        if(empty($value)) {
            $error=\MyApp\ErrorResponse::blankValue([$name]);
        }
        else {
            try {
                $stmt = $this->db->prepare("UPDATE charts SET $name=? WHERE id=? AND accounts_id=?");
                $stmt->execute([$value,$this->id,$this->account->id]);
            }
            catch(\PDOException $e){
                $error=\MyApp\ErrorResponse::duplicatedName();
            }
        }
        return isset($error)?$error:null;
    }
    protected function __updateType2($value,$name)
    {
        if(!$this->setParam($name, $value)) {
            $error=\MyApp\ErrorResponse::unknown();
        }
        return isset($error)?$error:null;
    }
    protected function __updateType3($value,$name)
    {
        if(!$this->setSubParam($name, $value)) {
            $error=\MyApp\ErrorResponse::unknown();
        }
        return isset($error)?$error:null;
    }
}

 

[requinix]

Do you have to use subclasses?

class Chart {

	private $type;

	private static $CHARTS = array(
		"bar" => array("xAxis", "yAxis"),
		"pie" => array(/* what do you need in a pie chart? */)
	);

	public function set($name, $value) {
		if ($name == "name") { // all charts have a name
			// update database
		} else if ($name == "title" || $name == "subtitle") { // all charts have a title and subtitle
			// ???
		} else if (in_array($name, self::$CHARTS[$this->type])) {
			// ???
		]
	}

}

$chart = new Chart(whatever, "bar");
$chart->set($_POST["attrName"], $_POST["value"]);

Sure, you could go the OOP route, but it's making this unnecessarily complicated when all the subclasses do (?) is indicate available field names.

[NotionCommotion]

I will be using subclasses for other needs, so if they help, I might as well use them.

 

Do you think I am on crack, or does the following make any sense?

$charts=new BarCharts();
$charts->updateParam($_POST['value'],$_POST['attrName']);  

class BarCharts extends Charts
{
    protected $allowedUpdateMethods=[
        'xAxis'=>'updateAxis',
        'yAxis'=>'updateAxis'
    ];
}

abstract class Charts
{
    protected $allowedUpdateMethods=[
        'name'=>'updateName',
        'title'=>'updateTitles',
        'subtitle'=>'updateTitles'
    ];

    protected function getAllowedMethods($name)
    {
        $allowedMethods=array_merge($this->allowedUpdateMethods,self::allowedUpdateMethods);    //I expect this needs a little work...
        return isset($allowedMethods($name))?$allowedMethods($name):'updateInvalid';
    }

    public function updateParam($value,$name)
    {
        return $this->{$this->allowedUpdateMethods($name)}($value,$name);
    }

    protected function updateName($value,$name)
    {
        // ...
        return $rs;
    }
    protected function updateTitles($value,$name)
    {
        // ...
        return $rs;
    }
    // This method really does not belong in all classes, but the above whitelists and prevents its use on non-applicable classes
    protected function updateAxis($value,$name)
    {
        // ...
        return $rs;
    }
    protected function updateInvalid($value,$name)
    {
        return ErrorResponse::invalidMethod();
    }
}

[requinix]

I'm not a fan of putting function or method names in strings (IDEs can't recognize them). And I'm not a fan of conventions like calling "update"+Name methods (too magical for my tastes). I'd still go with the "configuration"-type approach.

 

Unanswered question that would help to be answered:

 

What ways of handling values are there? For example, there's those three types from earlier: #1 was updating a column in a table, don't know about #2 and #3. I ask because my next thought is

abstract class Chart {

	private $fields;

	public function __construct() {
		$this->fields = $this->getFields();
	}

	protected function getFields() {
		return array(
			"name" => new UpdateType1("name", $this->id, $this->account->id),
			"title" => new UpdateType2(...),
			"subtitle" => new UpdateType2(...)
		);
	}

	public function set($name, $value) {
		if (isset($this->fields[$name])) {
			$this->fields[$name]->update($value);
		} else {
			error
		}
	}

}

class BarChart extends Chart {

	protected function getFields() {
		return array_merge(parent::getFields(), array(
			"xAxis" => new UpdateType3(...),
			"yAxis" => new UpdateType3(...)
		));
	}

}

interface IUpdateType {

	public function update($value);

}

class UpdateType1 implements IUpdateType {

	private $field;
	private $id;
	private $accountid;

	public function __construct($field, $id, $accountid) {
		$this->field = $field;
		$this->id = $id;
		$this->accountid = $accountid;
	}

	public function update($value) {
		// update database
	}

}

I forget the name of this design pattern. It's similar to Command.

[NotionCommotion]

I'm not a fan of putting function or method names in strings (IDEs can't recognize them). And I'm not a fan of conventions like calling "update"+Name methods (too magical for my tastes). I'd still go with the "configuration"-type approach.

 

Unanswered question that would help to be answered:

 

What ways of handling values are there? For example, there's those three types from earlier: #1 was updating a column in a table, don't know about #2 and #3. I ask because my next thought is

 

I forget the name of this design pattern. It's similar to Command.

 

 

I agree that calling "update"+Name methods is way to magical, however, my second (and this) example doesn't do so.  What do you mean by IDEs not being able to recognize them?

 

In regards to the unanswered question, I currently handle the values five different ways, but will likely add additional ways in the future, and wish the approach to scale.

 

Thanks for your help and interest!

class charts
{
    public function updateParam($name, $value)
    {
        /*
        $this->params must include array elements [name,value]
        name must be one of the following ['name', 'title', 'subtitle', 'xAxis', 'yAxis', 'type', 'theme'],
        however, not all chart types support each (i.e. pie doesn't support [xAxis', 'yAxis']).
        */
        $methods=$this->getAllowedUpdateMethods();
        $method=isset($methods[$name])?$methods[$name]:'updateInvalid';
        return $this->{$method}($value,$name);
    }

    /*
    Used with updateParam().  These are the default updateMethods, and child can add more
    */
    protected function getAllowedUpdateMethods()
    {
        return [
            'name'=>'updateName',
            'title'=>'updateTitles',
            'subtitle'=>'updateTitles',
            'type'=>'updateType',
            'theme'=>'updateTheme',
        ];
    }

    /*
    Used with updateParam().  Available to all.  Updates name in DB
    */
    protected function updateName($value,$notUsed)
    {
        if(empty($value)) {
            $error=\MyApp\ErrorResponse::blankValue(['Name']);
        }
        else {
            try {
                $stmt = $this->db->prepare("UPDATE charts SET name=? WHERE id=? AND accounts_id=?");
                $stmt->execute([$value,$this->id,$this->account->id]);
            }
            catch(\PDOException $e){
                $error=\MyApp\ErrorResponse::duplicatedName();
            }
        }
        return isset($error)?$error:["status"=>"success"];
    }
    /*
    Used with updateParam().  Available to all.  Updates first level config option
    */
    protected function updateTitles($value,$param)
    {
        $config=json_decode($this->config);
        if(!isset($config->{$param})) {
            $config->{$param} = new \stdClass();
        }
        $config->{$param}->text=$value;
        $this->saveConfigDB($config);
        return ["status"=>"success"];
    }

    /*
    Used with updateParam().  Available to only some chart types.  Updates second level config option
    */
    protected function updateAxis($value,$param)
    {
        $config=json_decode($this->config);
        if(!isset($config->{$param}->title)) {
            $config->{$param}->title = new \stdClass();
        }
        $config->{$param}->title->text=$value;
        $this->saveConfigDB($config);
        return ["status"=>"success"];
    }
    /*
    Used with updateParam().  Available to all.  Updates type
    */
    protected function updateType($value,$notUsed)
    {
        $sql="SELECT cth.id, cth.config, cth.type, cty.masterType
        FROM chart_themes cth INNER JOIN chart_types cty ON cty.type=cth.type
        WHERE cth.type =? LIMIT 1";
        return $this->updateTypeOrTheme($value,$sql);
    }
    /*
    Used with updateParam().  Available to all.  Updates theme
    */
    protected function updateTheme($value,$notUsed)
    {
        $sql="SELECT cth.id, cth.config, cth.type, cty.masterType
        FROM chart_themes cth INNER JOIN chart_types cty ON cty.type=cth.type
        WHERE cth.name =?";
        return $this->updateTypeOrTheme($value,$sql);
    }

    /*
    Used with updateParam() to respond to an invalid update method
    */
    protected function updateInvalid($value,$name)
    {
        return ErrorResponse::invalidMethod($name);
    }

}

class BarCharts extends Charts
{
    protected function getAllowedUpdateMethods()
    {
        return array_merge([
            'xAxis'=>'updateAxis',
            'yAxis'=>'updateAxis'
            ],parent::getAllowedUpdateMethods());
    }

    public function someOtherMethod() {}
}

[requinix]

What do you mean by IDEs not being able to recognize them?

If I need to do a code search for anything that calls the updateName() method then the IDE won't show me that getAllowedUpdateMethods() - because "updateName" is just a string value. It doesn't know it's a method name too.

Which is why PHP could benefit from a sort of ::function token like it did with ::class.

[NotionCommotion]

Ah, that makes sense.

 

Maybe I should rethink my whole approach?  Currently I am doing the following with a single endpoint:

curl -i -X PUT -H "Content-Type:application/json" http://localhost:8888
/charts/1 -d '{"name":"title","value":"My Title"}'

curl -i -X PUT -H "Content-Type:application/json" http://localhost:8888
/charts/1 -d '{"name":"xAxis","value":"My X Axis Title"}'

Maybe I should change to the following and have different endpoints?

curl -i -X PUT -H "Content-Type:application/json" http://localhost:8888
/charts/1/title -d '{"value":"My Title"}'

curl -i -X PUT -H "Content-Type:application/json" http://localhost:8888
/charts/1/xAxis -d '{"value":"My X Axis Title"}'

 

 

 

 

[requinix]

The first one is better. Note there's no particular reason why you couldn't have it support updating just one field at a time.

[NotionCommotion]

Both approaches do update just one field at a time.

 

The potential benefit of the second approach is a bar chart controller object could have a setXAxis() method but a pie chart controller object could not have one.

 

That being said, I am not stuck at all on doing so.  I am more concerned on implementing what is typical for a REST API.

[requinix]

Ah, yeah, I misread. I was thinking

curl -i -X PUT -H "Content-Type:application/json" http://localhost:8888/charts/1 -d '{"title":"My Title","xAxis":"My X Axis Title"}'

or I suppose

curl -i -X PUT -H "Content-Type:application/json" http://localhost:8888/charts/1 -d '{{"name":"title","value":"My Title"},{"name":"xAxis","value":"My X Axis Title"}}'

I would suggest that first one: it is more typical to represent an object with key/value pairs than with a {key,value} set, and generally means less processing involved for both the client and server.