Is it good/safe OOP practice to have a method whose only purpose is to send/retrieve data from another class?

Question

I have a class that performs basic MySQL operations. This is all in PHP.

class dbTables {
    public $name;
    protected $fields = array(); // array of dbTableField objects
    public $result_sets = array();
    protected $primary_key; // filled using getFields() from __construct()


    public function __construct($table_name) {
        $this->name = $table_name;
        $this->getFields();
    }
    public function removeWhitespace($table_name) {
        $this->name = $table_name;
        $this->getFields();
    }
    // etc...
}

So all you need is the table name to create the object, and construct automatically gets a bunch of field data from INFORMATION_SCHEMA (names, data types, etc).

I also have a method for removing whitespace from a column It's currently located in another class, but I'm thinking of moving into the above dbTables class. You can see the method below. I don't think it's really necessary to read the method to answer my question, but it could be helpful.

protected function removeWhiteSpace($target_field,$abbrev_field,$from_size,$to_size,$options) {
    // optional conditions based on original character length and resulting character length.
    /* options: from_size = all/longer_than/shorter_than
    *   to_size = all/longer_than/shorter_than
    */
    $temp_table = $this->table_name;        
    $sql = "UPDATE `$temp_table` 
        SET `undo_sku` = `$target_field`, `$target_field` = REPLACE(`$target_field`,' ',''), `$abbrev_field` = 'w'";
    $where = array();
        if($options['from_size'] == 'longer_than') {
            $where[] = "`$target_field` > $from_size";
        } else if($options['from_size'] == 'shorter_than') {
            $where[] = "`$target_field` < $from_size";
        }
        if($options['to_size'] == 'longer_than') {
            $where[] = "LENGTH(REPLACE(`$target_field`,' ','')) > $to_size";
        } else if($options['from_size'] == 'shorter_than') {
            $where[] = "LENGTH(REPLACE(`$target_field`,' ','')) < $to_size";
        }               
        //WHERE `$target_field` = '' AND LENGTH(REPLACE(`vnd_sku`,' ','')) <= $string_length";
    $result = mysql_query($sql);
}

Now, if I do move it into dbTables, I will still want to use the functionality in the other class. Should I just instantiate a dbTables object whenever I need to use removeWhitespace? Or, could I give dbTables its own removeWhitespace method like this:

protected function removeWhiteSpace($target_field,$abbrev_field,$from_size,$to_size,$options) {
  /* options: from_size = all/longer_than/shorter_than
  * to_size = all/longer_than/shorter_than
  */
  $dbtable = new dbTables($this->table_name);
  $temp_table = $this->table_name;

  $result = $dbtable->removeWhiteSpace($target_field,$abbrev_field,$from_size,$to_size,$options);
  return $result;
}

The advantage I see is that now I have a way to limit instantiation of the dbTables object in various other classes. Methods that need to use removeWhiteSpace can use $this->removeWhiteSpace instead of instantiating a dbTables object. The class's own removeWhiteSpace method takes care of that.

I'm kind of new to OOP, though, so I'm just wondering if that is good practice or bad.

I also hope I'm asking this question in the right place. I'm pretty sure it's not a StackOverflow question.

Answer 1

I think the code smell here is that you have multiple classes that hold table names.

Good object-oriented design calls for a class that wraps a table (which itself holds reference to class that wraps database connection) and using that to refer to tables in all the other classes.

I suspect that's what you are aiming at with the dbTables class. So yes, move the method there, but replace the table_name field in the other class with reference to dbTables instance, so the wrapper just says $this->table->removeWhitespace(...).

Answer 2

If you find yourself wanting to instantiate an object (e.g. dbTables) just to access one bit of very specific functionality in it, without actually using that object for what it is, then it's usually a code smell and a sign you need to portion up your code a little differently.

Why not factor out your removal of whitespace functionality, in a reusable and context-free a way as possible, into some sort of utility class? Then this can be accessed from all the places needing the functionality.

Alternatively, leave the function where it is, but make it static (class level), so that it can be called without using an instance of the class it's in.