The Config Class - Page 2

Village Idiot · 01-16-2009, 04:43 AM

Good work, but there is quite a bit that needs to be fixed. Here is what I want done (all small things):

Are so many comments really required (especially towards the beginning)? You heavily comment the declarations (which honestly is not necessary) yet leave out comments on the actual processing code.
Don't trigger errors when the script can still continue. Some procedure had changed and we are no longer going by exceptions, just return something to indicate failure.
Those constants are really not necessary.
Why are you assigning $this->query then using it once? Wouldn't it make more sense just to directly run the query?
Use parameters for the creation, dont make the programmer assign two class variables than run it.
This is a general database programming rule, always always always go by unique ID when possible. Make delete() delete by an ID given in a parameter.

Orc · 01-16-2009, 04:56 AM

Quote:

Originally Posted by Village Idiot

Good work, but there is quite a bit that needs to be fixed. Here is what I want done (all small things):

Are so many comments really required (especially towards the beginning)? You heavily comment the declarations (which honestly is not necessary) yet leave out comments on the actual processing code.
Don't trigger errors when the script can still continue. Some procedure had changed and we are no longer going by exceptions, just return something to indicate failure.
Those constants are really not necessary.
Why are you assigning $this->query then using it once? Wouldn't it make more sense just to directly run the query?
Use parameters for the creation, dont make the programmer assign two class variables than run it.
This is a general database programming rule, always always always go by unique ID when possible. Make delete() delete by an ID given in a parameter.

Done. (chars) check op

Orc · 01-16-2009, 05:03 AM

Updated again, fixed the grab options method.

Village Idiot · 01-16-2009, 05:05 AM

Much better, but still a few small things:

What is the purpose of &option?
Make name2id name_to_id. But why is the function necessary? Isn't the ID of the setting part of the returned array?
Why assign the parameters to the private variables then use them?

I think we had slightly different ideas on how to structure this (I did not make myself clear on this matter). I am not looking for any data to stay in the class past the function. The private variables are not necessary.

When these revisions are made, stick the code in the SVN repository and we can begin talking about the additions I want made to it.

Orc · 01-16-2009, 05:10 AM

Quote:

Originally Posted by Village Idiot

Much better, but still a few small things:

What is the purpose of &option?
Make name2id name_to_id. But why is the function necessary? Isn't the ID of the setting part of the returned array?
Why assign the parameters to the private variables then use them?

I think we had slightly different ideas on how to structure this (I did not make myself clear on this matter). I am not looking for any data to stay in the class past the function. The private variables are not necessary.

When these revisions are made, stick the code in the SVN repository and we can begin talking about the additions I want made to it.

I can get rid of the option since it pretty much has no function now except edit() which was using name2id for the option_id, maybe I could just make it where you can edit it by the id and just add the other two parameters and get rid of option() ?

Village Idiot · 01-16-2009, 05:13 AM

Quote:

Originally Posted by Orc

I can get rid of the option since it pretty much has no function now except edit() which was using name2id for the option_id, maybe I could just make it where you can edit it by the id and just add the other two parameters and get rid of option() ?

For ease of use, keep edit by name (thus keep name_to_id), but make the name, new name and new value in parameters.

Orc · 01-16-2009, 05:16 AM

Quote:

Originally Posted by Village Idiot

For ease of use, keep edit by name (thus keep name_to_id), but make the name, new name and new value in parameters.

Ahh I get it. okay

Village Idiot · 01-16-2009, 05:17 AM

Sweet, once you do that SVN it and we can get started on the additions.

Orc · 01-16-2009, 05:18 AM

Hows this:

PHP Code:


			
class TalkPHP_Config {

    public static $configinfo = array();
    protected $dbobj;

    public function __construct($dbObj) {

        if ( is_object($dbObj) && $dbObj instanceof DBMysql )
        {
            $this->dbobj = $dbObj;
        } else throw new Exception( "Database Object failed, error called." );

        self::grabOptions();

    }

    /**
     * Create Options
     *
     * @return boolean
     */
    public function create($optionname, $optionvalue)
    {
        
        if ( isset($optionname)
         &&  isset($optionvalue) )
        {
            $optionname  = $this->dbobj->secure($optionname);
            $optionvalue = $this->dbobj->secure($optionvalue);

            $this->dbobj->exeQuery("INSERT INTO `options` VALUES('','$optionname','$optionvalue');");

            return true;

        }

        return false;

    }

    /**
     * Remove Options
     *
     * @return boolean
     */
    public function remove($optionid)
    {

        if ( is_numeric($optionid) )
        {
            
            $this->dbobj->exeQuery("DELETE FROM `options` WHERE option_id = $optionid");
            return true;
            
        }

        return false;
    }

    /**
     * Edit Options
     *
     * @return boolean
     */
    public function edit($name, $newname, $newvalue)
    {
        if ( isset($name)
          && isset($newname)
          && isset($newvalue) )
        {
            $id = $this->name_to_id($name);
            $newname  = $this->dbobj->secure($newname);
            $newvalue = $this->dbobj->secure($newvalue);
            
            $this->dbobj->exeQuery("UPDATE `options`
            SET `option_name` = '$newname', 
            `option_value` = '$newvalue'
            WHERE option_id = $id");

            return true;

        }
        
        return false;
    }

    /**
     * Grab all the Options
     *
     */
    private function grabOptions()
    {
        $options = $this->dbobj->exeQuery("SELECT option_id, option_name, option_value FROM `$this->table`");

        if ( $options->rows() > 0 )
        {

            while ( $options->getFetch(false, true) )
            {
                
                $this->configinfo["$options[option_name]"] = $options[option_value];
                
            }

            return true;

        }
        
        return false;

    }

    /**
     * Converts the Name to the Id of the settings
     * 
     * @return int
     */
    private function name_to_id($name)
    {

        if ( isset($name) )
        {
            $name = $this->dbobj->secure($name);

            $name_to_id = $this->dbobj->exeQuery("SELECT option_id, option_name FROM `options` WHERE option_name = '$name'");

            if ( $name_to_id->rows() > 0 )
            {
                $name_to_id->getFetch(false, true);

                return $name_to_id["option_id"];
            }

        }

        return false;
    }

}

Orc · 01-16-2009, 05:19 AM

Quote:

Originally Posted by Village Idiot

Sweet, once you do that SVN it and we can get started on the additions.

I thought we were using google code?

Village Idiot · 01-16-2009, 05:21 AM

Quote:

Originally Posted by Orc

I thought we were using google code?

Google uses SVN for source control.

And the function looks great.

Orc · 01-16-2009, 05:22 AM

Quote:

Originally Posted by Village Idiot

Google uses SVN for source control.

And the function looks great.

Oh okay, sorry I don't know much about it. > . <

Orc · 01-16-2009, 05:24 AM

Can you link me to the SVN. ;P

Village Idiot · 01-16-2009, 05:28 AM

SVN is a program on your computer that interacts with the server, read this tutorial
http://internetducttape.com/2007/03/...n_tortoisesvn/

SVN is a little hard to get at first, but makes tons of sense once you get it.

Orc · 01-16-2009, 05:30 AM

Quote:

Originally Posted by Village Idiot

SVN is a program on your computer that interacts with the server, read this tutorial
http://internetducttape.com/2007/03/...n_tortoisesvn/

SVN is a little hard to get at first, but makes tons of sense once you get it.

I know what it is, but I think i know what you're saying yeah. I've done this before with android. :P

Tanax · 01-16-2009, 09:23 AM

Some minor details about 2 of your functions that I don't think will work with the SQL class. Concerns the graboptions and name2id functions.

graboptions:
Looks like this currently

PHP Code:


			
private function grabOptions()

    {

        $options = $this->dbobj->exeQuery("SELECT option_id, option_name, option_value FROM `$this->table`");



        if ( $options->rows() > 0 )

        {



            while ( $options->getFetch(false, true) )

            {

                

                $this->configinfo["$options[option_name]"] = $options[option_value];

                

            }



            return true;



        }

        

        return false;



    }

- The $options variable is pretty useless, since the assignment to it comes from the result of exeQuery, which only returns $this, which basicly means that you're assigning $options = $this->db;. You don't need the $option variable there.

- The rows function is called getRows

- The while loop looks a little suspicious, not sure if it would work. If you check the SQL class, you see that when I fetch the array, I already loop it there with a while loop. So you don't need to use a while loop in your code, you can skip it. Use a foreach instead to get the results.

- You should always use ' whenever you're checking a specific value in an array: $options['option_value']

- If you're checking a value inside an array, against a variable, I don't think it's neccessary to use ": $this->configinfo[$options['option_value']]

Something like this would be more accurate I think:

PHP Code:


			
private function grabOptions()

{

    $sql = "SELECT option_id, option_name, option_value FROM `$this->table`";



    if ( $this->dbobj->getRows($sql) > 0 )

    {



        $options = $this->dbobj->getFetch(false, true);



        foreach( $options as $option )            

        {

                

            $this->configinfo[$option['option_name']] = $option['option_value'];

                

        }



        return true;



    }

        

    return false;



}

But.. haven't tested so I don't know. It should be more correct with the SQL class however.

Tanax · 01-16-2009, 09:29 AM

Name2Id:
Looks currently like this

PHP Code:


			
private function Name2Id()

    {



        if ( isset($this->sSettingname) )

        {

            $this->sSettingname = $this->dbobj->secure($this->sSettingname);



            $name2id = $this->dbobj->exeQuery("SELECT option_id, option_name FROM `$this->table` WHERE option_name = '$this->sSettingname'");



            if ( $name2id->rows() > 0 )

            {

                $name2id->getFetch(false, true);



                return $name2id["option_id"];

            }



        } //else trigger_error( self::ERR_SETTINGS_NAME_EMPTY,  E_USER_ERROR );



        return false;

    }

- Same here about the assigning of $name2id like on the $options. You're assigning the dbobject to the $name2id, which is okey, but why?

- Rows function is called getRows as said before

Something like this would be more accurate:

PHP Code:


			
private function Name2Id()

{



    if ( isset($this->sSettingname) )

    {



        $this->sSettingname = $this->dbobj->secure($this->sSettingname);

        $sql = "SELECT option_id, option_name FROM `$this->table` WHERE option_name = '$this->sSettingname'";



        if ( $this->dbobj->getRows($sql) > 0 )

        {

                

            $array = $this->dbobj->getFetch(false, true);



            return $array['option_id'];

            

        }



    } //else trigger_error( self::ERR_SETTINGS_NAME_EMPTY,  E_USER_ERROR );



    return false;

    

}

But as said before, haven't tested so I don't know if this would work. It should however be more accurate with the SQL class.

Orc · 01-16-2009, 09:42 AM

Quote:

Originally Posted by Tanax

Name2Id:
Looks currently like this

PHP Code:


			
private function Name2Id()

    {



        if ( isset($this->sSettingname) )

        {

            $this->sSettingname = $this->dbobj->secure($this->sSettingname);



            $name2id = $this->dbobj->exeQuery("SELECT option_id, option_name FROM `$this->table` WHERE option_name = '$this->sSettingname'");



            if ( $name2id->rows() > 0 )

            {

                $name2id->getFetch(false, true);



                return $name2id["option_id"];

            }



        } //else trigger_error( self::ERR_SETTINGS_NAME_EMPTY,  E_USER_ERROR );



        return false;

    }

- Same here about the assigning of $name2id like on the $options. You're assigning the dbobject to the $name2id, which is okey, but why?

- Rows function is called getRows as said before

Something like this would be more accurate:

PHP Code:


			
private function Name2Id()

{



    if ( isset($this->sSettingname) )

    {



        $this->sSettingname = $this->dbobj->secure($this->sSettingname);

        $sql = "SELECT option_id, option_name FROM `$this->table` WHERE option_name = '$this->sSettingname'";



        if ( $this->dbobj->getRows($sql) > 0 )

        {

                

            $array = $this->dbobj->getFetch(false, true);



            return $array['option_id'];

            

        }



    } //else trigger_error( self::ERR_SETTINGS_NAME_EMPTY,  E_USER_ERROR );



    return false;

    

}

But as said before, haven't tested so I don't know if this would work. It should however be more accurate with the SQL class.

He doesn't want private members

Tanax · 01-16-2009, 12:10 PM

In your class, no.
But what does that have to do with my edits of the functions in your class?

I'm not using private members in your functions. I just edited the SQLclass usage part to make it work with my class

Orc · 01-16-2009, 12:13 PM

Quote:

Originally Posted by Tanax

In your class, no.
But what does that have to do with my edits of the functions in your class?

I'm not using private members in your functions. I just edited the SQLclass usage part to make it work with my class

Sorry, it's late and a bit burnt out. Thank you for clearly that up for me.

appreciate it!