Solved

opinion on how my script is programmed (codeigniter)

Posted on 2010-11-08
4
317 Views
Last Modified: 2012-06-27
Hi,

I need your opinion on:
if my script is well programmed. It is made in codeigniter.

Before I continue, sorry for this long post.

As I previously stated in another post I'm just a rookie.
So I wanted to ask you guys what you think of my application so far.

Is it well written? How about the placement of the files/code?
What would you do?

The following code has the basics ofhttp://www.phpandstuff.com/articles/codeigniter-doctrine-from-scratch-day-1-install-and-setup

Oke so lets get started:
My application is a start of a cms system: Simple admin login and forgot password. When you forgot the password, a random new password will be emailed.

Login screen:
I created a table user for both the admins and users... I make the difference using a "user level" - 1 being the admin, 2 or above the normal users.
- When the form is being submit, the "submit" function is being called.
- In the submit function we have a "_submit_validate" function with form validations
- In the "_submit_validate" function we have a callback to authenticate

authenticate function in the login controller:
    public function authenticate() {

        $isAuthenticated = Current_User::login($this->input->post('username'),
                                    $this->input->post('password'));
        $isAdmin = Current_User::user()->ulevel;
        if ($isAuthenticated == TRUE && $isAdmin == 1) {
            return TRUE;
            }
        return FALSE;
    }

Open in new window


Instead of duplicating the Current_User::login() model function, I kept the same code and added a user level check.

fyi in the user model I added
$this->hasColumn('ulevel', 'integer', 5);

Open in new window

in the setTableDefinition function.

And now for the diffcult part...

Forgot Password:
I have a simple form with one email imputbox and a submit button.
- When the form is being submit, the "submit" function is being called.
- In the submit function we have a "_submit_validate" function with form validations
- In the "_submit_validate" function we have a callback to forgot_password

forgot_password and _send_password from the forgot_password controller:
public function forgot_password() {

        $pass = Current_User::forgot_password($this->input->post('email'));
        $this->_send_password($pass, $this->input->post('email'));
    }
    private function _send_password($pass, $email) {

        $this->load->library('email');
        $this->email->from('admin@aptest.com', 'apTest');
        $this->email->to($email); 
        
        $this->email->subject('Reset Password - apTest');
        $this->email->message('Here is your new password: ' & $pass);    
        
        $this->email->send();        
    }

Open in new window


current_user model (partial):
public static function forgot_password($email) {
        if ($u = Doctrine::getTable('User')->findOneByEmail($email)) {
            $newPassword = Current_User::user()->reset_password();            
            $u->password = $newPassword;
            $u->save();
            return $newPassword;
        }
        return FALSE;
    }

Open in new window


user model (partial):
    public function Reset_Password() {
        return $newPassword = random_string('numeric', 16);
    }

Open in new window


Now I'm kind of lost, if this is well programmed or not?. What do you think? Any opinion is appreciated :)

Kind regards
0
Comment
Question by:Mutsop
  • 2
4 Comments
 
LVL 17

Accepted Solution

by:
sweetfa2 earned 250 total points
ID: 34091553
Ok, lets go:

Authenticate function - you are not actually authenticating (ie. you are really authenticating admin - so call it adminAuthentication if that is what you want it to do).  Good programming practice is to create a method to authenticate.  A separate function is to check if in admin role.  So authentication v. authorisation are really two different functions.

Apart from that - it doesn't seem to bad.
0
 
LVL 13

Assisted Solution

by:darren-w-
darren-w- earned 250 total points
ID: 34092392
Just a little code refactoring:

with:
 public function forgot_password() {

        $pass = Current_User::forgot_password($this->input->post('email'));
        $this->_send_password($pass, $this->input->post('email'));
    }

break it out into two methods

public function forgot_password() {
        $this->_send_password($this->getPassword(), $this->input->post('email'));
    }

private function getPassword(){
      return Current_User::forgot_password($this->input->post('email'));
}

Also:

public static function forgot_password($email) {
        if ($u = Doctrine::getTable('User')->findOneByEmail($email)) {
            $newPassword = Current_User::user()->reset_password();            
            $u->password = $newPassword;
            $u->save();
            return $newPassword;
        }
        return FALSE;
    }

to

public static function forgot_password($email) {
        if ($u = Doctrine::getTable('User')->findOneByEmail($email)) {          
            $u->password =$this->getNewPassword();
            $u->save();
            return $u->password;
        }
        return FALSE;
    }


private function getNewPassword() {
return Current_User::user()->reset_password();
}


Sure more can be done...


0
 
LVL 3

Author Comment

by:Mutsop
ID: 34092770
@sweetfa2:
So you mean splitting the authentication part and checking for if that authenticated member has the rights to view a certain page (like an admin level)?

@darren-w-:
I do indeed need to refactor most of the script, problem is I don't quite know how and what should be script apart.
0
 
LVL 17

Expert Comment

by:sweetfa2
ID: 34095208
Yes that is what I mean.
0

Featured Post

Free Tool: IP Lookup

Get more info about an IP address or domain name, such as organization, abuse contacts and geolocation.

One of a set of tools we are providing to everyone as a way of saying thank you for being a part of the community.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Suggested Solutions

I imagine that there are some, like me, who require a way of getting currency exchange rates for implementation in web project from time to time, so I thought I would share a solution that I have developed for this purpose. It turns out that Yaho…
Part of the Global Positioning System A geocode (https://developers.google.com/maps/documentation/geocoding/) is the major subset of a GPS coordinate (http://en.wikipedia.org/wiki/Global_Positioning_System), the other parts being the altitude and t…
The viewer will learn how to dynamically set the form action using jQuery.
The viewer will learn how to look for a specific file type in a local or remote server directory using PHP.

789 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question