2
\$\begingroup\$

I am trying to manage an "app list" in my app (I currently have "MS Office" app list, "Games" app list and "other" app list).

lists table:

list_id     |   list_name 
----------------------------
1           |   MS Office 
2           |   Games       
3           |   Other

app to list table:

app_id      |   list_id 
----------------------------
1           |       1
2           |       1
3           |       1
4           |       3
5           |       2

I want to create an object for each list and to be able to fetch data from each list (Unless it's not recommended due to bad practice), practicing OOP.

Lets say I summon the MS Office list - $office_list = new List_ms_office(); and get an object that handles the ms office list.

I am adding my working code, I was just wondering if it's written well, structured well, doesn't have any "bad practices", ect'.

This is my Main List class:

<?php
namespace App\Models;
use App\Core\Database;


 /**
  *
  *  Static List Template: Class that controls all data to and from `list_%`
  *
  */
class StaticList {


/*=================================
=            Variables            =
=================================*/

    # Database instance
    private $db;
    # Result app list array 
    protected $appList;
    # Developer must include the list name of the llist
    // abstract protected $listName;


/*===============================
=            Methods            =
================================*/


    /**
     *
     *  Construct
     *  Init DB connectiom
     * 
     */
    public function __construct($listName){
        # Get database instance
        $this->db = Database::getInstance();
    }


    /**
     *
     *  Get List ID: Gets the list id by the list name (from 'lists' table)
     *  @param      $listName   String      List name 
     *  @throws                 Init        Returns the ID of the list 
     *
     */
    private function get_list_id_by_name($listName) 
    {
        $sql            = "SELECT `list_id` FROM `lists` WHERE `list_name` LIKE :listName";
        $list_result    = $this->db->queryIn($sql, array("listName" => "%{$listName}%"));

        return $list_result[0]['list_id'];
    }


    /**
     *
     *  Get list array by id
     *  @param      $list_id    init    Gets a list id (as set in the 'list_apps' table)
     *  @throws                 Array   Returns an array of apps (which is the list)
     *
     */
    private function get_list_array_by_id($list_id)
    {
        $app_query = "SELECT `app_id` FROM `list_apps` WHERE `list_id` = :listID";
        return $this->db->query($app_query, array('listID'=>$list_id));
    }


    /**
     *
     *  Get List of Apps
     *  @param      $listName   String      Gets a list name 
     *  @throws                 Array       Returns app list as an array
     *  
     */
     protected function get_list_apps($listName)
     {
        # Get the list ID by List Name 
        $list_id    = $this->get_list_id_by_name($listName);

        # Get list array by id 
        $app_result = $this->get_list_array_by_id($list_id);

        # Get list organized and ready
        foreach ($app_result AS $key => $app_array) {
            $app_id_array[] = $app_array['app_id'];
        }

        # Genrerate object
        $App        = new App();
        $app_data   = $App->get_multiple_apps_data($app_id_array);

        return $app_data;
    }
}

And this is my list extension (in this specific case, it's an MS Office class - fetching the list):

<?php
namespace App\Models\Lists;
use App\Core\Database;
use App\Models\StaticList;

/* 
 * App List: MS Office
 */
Class List_ms_office extends StaticList {

/*=================================
=            Variables            =
=================================*/

    # Database instance 
    private $db;
    # This list must have a name right? 
    private $listName = 'MS Office';


/*===============================
=            Methods            =
================================*/

    /**
     *
     *  Construct
     *  Init DB connectiom
     * 
     */ 
    public function __construct() {
        $this->db       = Database::getInstance();
        // $this->appList   = $this->get_list_apps($this->listName);
    }


    /**
     *
     *  Get List: Gets the list of this object
     *
     */
    public function getList()
    {
        $this->appList  = $this->get_list_apps($this->listName);
        return $this->appList;
    }


}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ You have a few questions asked at this site and it looks like the answers you have received have been helpful to you. This site goes both ways. If you receive help you should reward those who help you by accepting their answer. Read How does accepting an answer work? and start giving back to the community. You also get two reputation points for each accepted answer! :) \$\endgroup\$
    – John Conde
    Commented Jan 9, 2019 at 2:07

1 Answer 1

1
\$\begingroup\$

I think the biggest issue here is not properly working with abstractions.

Before we can create a proper abstraction, we need a better name. When I first looked at the "StaticList" class, I assumed all members of this class were static, as in, not an instance member. They are all instance members, which makes the name of the class misleading. Calling this class "ApplicationList" would be more appropriate

So, first things first, rename the StaticList class (I'll use ApplicationList from here on).

Abstraction #1: An ApplicationList has a name

You created a child class that is specific to a single list and hard coded the name of the list. Move this name up into the ApplicationList class and pass the name in when instantiating the list at runtime:

$list = new ApplicationList('MS Office');

Abstraction #2: iterating over the list

Since you are modeling a list, and I'm sure at some point you'll want to iterate/loop over all the items in the list, you can have ApplicationList implement the Iterator interface in PHP, so you can do things like:

foreach ($list as $index => $item) {
    // do something with $item
}

Separation of Concerns

You are also mixing data access code with an object more focused on business logic. Consider moving this logic into another class.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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