Values being returned as NULL

Andrew · 10-24-2007, 05:11 AM

I'm not sure what's causing it, I'm using a bit of the code from when you guys helped me last which seemed work work, but now it's not.

http://phpfi.com/271062

For the elseif section to see if any variables are empty, would that be best to put into one if condition and use the || operator?

Also, I have another question. Is all that in the __construct() function necessary? It doesn't seem like it, but I'm not sure how else to pass the variables off into member variables.

Karl · 10-24-2007, 10:00 AM

Hi Andrew,

The class looks fine to me. Setting default values in the constructor is a good thing, it provides a consistent place for property initialisation. With that said, if not all properties need to be set via the construct call then I would suggest adding a few "settor" methods, for example, to set the title independently of the constructor then you could add this method:

PHP Code:


			
public function setTitle($szTitle)
{
    $this->m_szTitle = $szTitle;
}

If you need to access any of these properties outside the class you should also create "gettor" methods to do this, such as:

PHP Code:


			
public function getTitle()
{
    return $this->m_szTitle;
}

Both the gettor and settor methods are only any use when you make your class properties hidden from outside the scope of the class (private or protected), they allow users to change the values of your properties but only by using the rules that you specify in you settor. I would suggest this change as an improvment.

As for your santize method, this isn't the cleanest approach to validation but it looks OK for its the task it is supposed to do.

Andrew · 10-28-2007, 08:20 PM

How would you recommend doing the santize function? Not looking for you to create a full function to do it, but could you list off how you would go about doing it?

bluesaga · 10-29-2007, 10:14 AM

I think what Karl is refering too, is that you do not have any clarity in your error reporting:

PHP Code:


			

        if (empty($this->m_name) || empty($this->m_email) || empty($this->m_artist) || empty($this->m_album) || 
            empty($this->m_release) || empty($this->m_song) || empty($this->m_lyrics)) {
            $szError .= 'Please fill in all required fields.<br />'."\n";
            $bError = true;
        }

Errors like "You did not fill in the lyrics column" would be better.

I would hazard a guess that Karl will recommend an external validation class, as would i :)

Karl · 10-29-2007, 11:55 AM

Yep bluesaga is right. I would improve the validation to allow for custom error messages, as bluesaga said, an external, reusable class would be ideal for this job.

Andrew · 10-30-2007, 12:45 AM

Well, I would like to get it working first, then update later. I'll work on the specified errors for now.

Fixed all the bugs. Just stupid errors like forgetting the m_, capitalizing stuff that shouldn't have been, etc.