PHP OOP need advice

Question

i am building this sms notification system, which will send 10 times free sms based on certain occasion to the web's member, and after a certain member reach 10 times, the system would send a last notification system saying that "this is the last free sms notification", i am currently learning PHP OOP and trying to use an OOP aproach on this

without further a do here's my code:

<?php
class SmsBonus {
//bonus_sms fields = id, member_id, counter, end_status

public static function find_member($id=0){
    //query to find a certain member
}

public function add_counter($id=0){
    //query to increment the value of counter field
}

public function status_check($id=0){
    //query to check whether the given member's counter has reach the number 10
}

public static function send_sms($id, $message){ 
    $found = $this->find_member($id);
    $status_check = $this->status_check($id);

    if(!empty($found) && !empty($status_check) && $found->counter == 10){
        //send the sms notification saying that this member has reach the end of the bonus period

        //update this member's end_status table to 1
    }else{
        //send the regular notification 
    }
}

}
?>

would this lines:

$found = $this->find_member($id);
$status_check = $this->status_check($id);

work as expected (i cant test this one out because i am currently building this on local)? and is this a best practice regarding OOP aproach ? or am i doing this wrong ?

i need advice, thank you very much.

EDIT:

of course on my original code i declare the class, i am sorry that by not writing it here confuses everybody :D, i am actually looking for a kind of answer (advice) that pointed the way i should implement a best approach (best practice) on my codes (in this case methods), things that i worry about is that i don't meet the requirements such as K.I.S.S or D.R.Y.

UPDATE i manage to do some modifications based on your suggestions, how is this looks like ?

<?php
    class SmsBonus{
        //bonus_sms fields = id, member_id, counter, end_status
        protected $max_sms = 10;

        public $id;
        public $member_id;
        public $counter;
        public $end_status;

        public function find_member($id=0){
            //query to find a certain member
        }

        public function add_counter($id=0){
            //query to increment the value of counter field
        }

        public function status_check($id=0){
            //query to check whether the given member's counter has reach the number 10
        }


        public function update_status($id=0){
            //query to update when a certain member reach its sms bonus limit
        }

        protected function can_still_send_sms($member_id){
            $found          = $this->find_member($member_id);
            $status_check   = $this->status_check($id);
            return !empty($found) && $found->counter < $this->max_sms && !empty($status_check);
        }

        public function send_sms($id, $message){
            $phone  = Phone::find_member($id); //
            if ($this->can_still_send_sms($id)) {       
                //send the sms notification saying that this member has reach the end of the bonus period

                $this->update_status($id);
            }else{              
                //send the regular notification 

                $this->add_counter($id);
            }
        }
    }
    $sms_bonus = new SmsBonus();
?>

Spiny Norman · Accepted Answer · 2010-12-21 10:20:49Z

Well, I think OOP is mostly about creating meaningful actions that are easy to reuse and, especially, make it easy to find out what's going on when you revisit your code some months later (or when someone else reads your code, which is more or less the same). Also, when you found your member, you can then perform logic on that, instead of on the id. So, in this case it might be better to create your methods like this, for example:

protected $max_sms_messages = 10;

protected function can_still_send_sms($member){
    return !empty($member) && $member->counter < $this->max_sms_messages;
}

public function send_sms($id, $message){ 
    $found = $this->find_member($id);
    if ($this->can_still_send_sms($found)) { // or even if($found->can_still_send_sms()), if you want to implement it that way

        //send the sms notification saying that this member has reach the end of the bonus period

        //update this member's end_status table to 1
    }else{
        //send the regular notification 
    }
}

Also, for the record, you can't call non-static methods from static methods.

dewy · Accepted Answer · 2010-12-21 10:41:21Z

1

You need to wrap your code in a class declaration

class SMSNotification {
...
}

And you may also want to create a constructor

function __construct() {

One reason for this is so that you can set private variables to the class when it is instantiated.

you instantiate the class like this:

$sms = SMSNotification()

Will you be connecting this to a database for the counter increment. As what you would normally do with an oop approach is have a seperate class that handles that connection, so that if you wanted to build on this whole project then everything would be connecting to a database the same way.

The two lines of code you pasted are a bit different:

$found = $this->find_member($id);

you have made find_member a static function (which is probably what i would have done) so that you can call the function without create a new class object. That being said it is not of value $this as it is not part of the current instantiated class. So you will need to call it like (using my example of SMSNotification):

$found = SMSNotification::find_member($id);

This tells php to look for a static function called find_member

The other line of code should work fine:

$status_check = $this->status_check($id);

edited Dec 21, 2010 at 10:41

answered Dec 21, 2010 at 10:13

dewy

564 bronze badges

3 Comments

littlechad Over a year ago

i did declare the class, i just didn't write that on my question, and i also know how to instantiate, but thanks for pointing out :D

dewy Over a year ago

okay sure, just assumed cause you said "here's my code". You dont want to put the 2 brackets when declaring a class like how you have above "class SmsBonus(){" should be "class SmsBonus{"

littlechad Over a year ago

oh yeah, typo i was in a hurry :p

jwueller · Accepted Answer · 2010-12-21 10:38:15Z

0

According to OOP you can't call $this->find_member($id) on a static member function. Besides you didn't declare any class at all, so $this is meaningless (as far as I remember PHP). You probalby wanted to declare some SmsClient class which would be initialized from db query filling in member variables. Your static find_member($id=0) function would query db by id and return initialized object of SmsClient with its id = $id

class SmsClient
{
 private $id;
 private $nSmsSent;

 public __construct($id)
 {
  $res = DAL->GetClient($id);
  //initialize vars here
 }

 public send_sms(...)
 {
  $this->nSmsSent++;
 }
}

edited Dec 21, 2010 at 10:38

jwueller

31.1k5 gold badges69 silver badges71 bronze badges

answered Dec 21, 2010 at 10:13

Nickolodeon

2,9663 gold badges26 silver badges42 bronze badges

Comments

jwueller · Accepted Answer · 2010-12-21 10:39:08Z

0

Listen to dewi.

Anyway, a nice way to test if you're using the right syntax is commenting out the content of the find_member() and status_check() functions and make them return some arbitrary values: if the values are actually returned, you're doing it right.

edited Dec 21, 2010 at 10:39

jwueller

31.1k5 gold badges69 silver badges71 bronze badges

answered Dec 21, 2010 at 10:14

cbrandolino

5,8932 gold badges21 silver badges27 bronze badges

Collectives™ on Stack Overflow

PHP OOP need advice

4 Answers 4

Comments

3 Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

Comments

3 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related