2
\$\begingroup\$

I have the following function for validating users facebook information against the rules setup in the database for users. Its working fine but i need to know if it can be more optimized. Few things that you need to know are:

  1. There is a possibility that particular rule would not exists in db.
  2. But if exists and does not match with user details it must redirect to error page.
  3. it is also possible that user has not set the information in their profile so in this case it should redirect to error page.
  4. my dbdetails array has following structure is

    Array( ['Demography'] => Array( ['Rule'] => Array( [0] => 'Some Value' [1] => 'Some Value'... ) ['Value'] => Array( [0] => 'Some Value' [1] => 'Some Value' ) ) )

And the fbdata is exactly same what facebook returns when user visits the page after allowing the application. Here is my function which validates/

private function validate_demographics($dbDetails, $fbData)
    {
        $this->load->library('facebook', array('appId' => APP_ID, 'secret' => APP_SECRET));

        //Is page like restriction is there
        if( in_array('People Who Like My Page', $dbDetails['Demography']['Value']) )
        {
            //Checks whether user has liked the page or not
            if( !$fbData['page']['liked'] ) {
                //If user has not liked the page set the network flag to 1.
                redirect('viewer/demographics_not_match/1');
            }
        }

        if( in_array('Age', $dbDetails['Demography']['Rule']) ) {
            $ageInd = array_search('Age', $dbDetails['Demography']['Rule']);
            $validAge = explode('-', $dbDetails['Demography']['Value'][$ageInd]);
            $userBirthdate = explode('/',$this->viewer_fb_data['birthday']);
            $userAge = date('Y') - $userBirthdate[2];
            if( $userAge >= $validAge[0] && $userAge <= $validAge[1] ) {
                return true;
            } else {
                redirect('viewer/demographics_not_match/2');
            }
        }

        if( in_array('High-School', $dbDetails['Demography']['Rule']) or in_array('College', $dbDetails['Demography']['Rule']) ) {
            //Checks whether user has set his educational details on facebook
            if( array_key_exists('education', $this->viewer_fb_data) ) {
                foreach( $this->viewer_fb_data['education'] as $userEducation )
                {
                    if( in_array($userEducation['school']['name'], $dbDetails['Demography']['Value']) ) {
                        return true;
                    }
                }
            } else {
                redirect('viewer/demographics_not_match/3');
            }
        }

        if( in_array('City', $dbDetails['Demography']['Rule']) or in_array('State', $dbDetails['Demography']['Rule']) or in_array('Country', $dbDetails['Demography']['Rule']) )
        {
            //Checks whether user has set his location on facebook
            if( array_key_exists('location', $this->viewer_fb_data) ) {
                $userLocation = explode(',', $this->viewer_fb_data['location']['name']);
                if( in_array($userLocation[0], $dbDetails['Demography']['Value']) ) {
                    return true;
                }
            } else {
                redirect('viewer/demographics_not_match/5');
            }
        }

        if( in_array('Work-Place', $dbDetails['Demography']['Rule']) )
        {
            //Checks whether user has set his work details on facebook
            if( array_key_exists('work', $this->viewer_fb_data) ) {
                $workInd = array_search('Work-Place', $dbDetails['Demography']['Rule']);
                $workPlaces = explode(',', $dbDetails['Demography']['Value'][$workInd]);
                foreach( $this->viewer_fb_data as $fbWork )
                {
                    if( in_array($fbWork['employer']['name'], $workPlaces) ) {
                        return true;
                    }
                }
            } else {
                redirect('viewer/demographics_not_match/4');
            }
        }
        return false;
    }
\$\endgroup\$
  • \$\begingroup\$ Hi! Thanks for your question. You're asking for a performance improvement, but you didn't say how slow it was, and where you think the issue is. "Premature optimization is the root of all evil." Did you consider using software like XDebug too locate possible bottlenecks? If you also want a more traditional code review, then ask for it. :) \$\endgroup\$ – Quentin Pradet Mar 8 '12 at 9:03
  • \$\begingroup\$ i haven't face any issue yet. Just need senior advise if anything i have missed or is there any overhead i have created in this code. Since this code needs to handle around 10,000 users simultaneously when the project goes live so i dont how it behaves at that time. \$\endgroup\$ – Shayan Husaini Mar 8 '12 at 9:08
  • \$\begingroup\$ Your controller seems to be doing an awful lot of work, this is often indicative that you're implementing business logic in a controller. This is an antipattern known as "fat controller". Business logic belongs in your models, the controllers should only really contain glue logic and be "thin" (They just pass user input to the model and model output to the view without making any business-logic decisions). \$\endgroup\$ – GordonM Mar 8 '12 at 17:55
  • \$\begingroup\$ yeah i have often heard that apply the business logic in a model. But i dont find any disadvantages of doing that if you can point out then please let me know.... \$\endgroup\$ – Shayan Husaini Mar 9 '12 at 6:28
  • \$\begingroup\$ @ShayanHusaini If the business logic is in the model, then the model becomes easy to reuse as it's pretty much stand-alone. You can plug it into any controller and action you want. If a big chunk of the business logic is in the controller then the model can't be reused without copying and pasting code from its associated controller. Besides, semantically speaking it's the model's job to implement business logic because a model embodies a thing or a concept in your system and it should know in what ways it can be expected to be used. \$\endgroup\$ – GordonM Mar 9 '12 at 7:40
1
\$\begingroup\$

The code looks OK, and what is probably going to take you time is the call to the Facebook API, which means there's no need to optimize that snippet.

I also have two unimportant remarks:

  • Your function either returns true/false, or redirects to another page. There is no reasonable way to guess that (given a name such as validate_demographics). You should be careful when you are redirecting in some cases but not all of them. This is a lack of consistency that could prove dangerous afterwards (eg. if you put a redirect after this is executed, it won't work in every case).
  • The validation method is a bit surprising: as soon as you found a rule that is validated, your return TRUE. Don't you want to check that all the rules match?

You want to consider it valid when one of the rules matches, and invalid when all them don't match. If nothing matches, where do you want to redirect the user? #1? #4?

To solve the "does one rule match?" problem, I would set a variable $nothing_matches to TRUE at the beginning of the function, and change every return TRUE to $nothing_matches = FALSE. You can simply return $nothing_matches; at the end of the function.

Now, if nothing matched (`$nothing_matched is TRUE) you can hardcode the rule you want to redirect to, since there's no better way to choose the rule.

\$\endgroup\$
  • \$\begingroup\$ currently the scenario is if any of the information is matched with the given criteria then user will be supposed as valid user. \$\endgroup\$ – Shayan Husaini Mar 8 '12 at 13:06
  • \$\begingroup\$ Not really. If the first one is wrong, it redirects, and the others won't be evaluated (since redirect() calls exit;. You might want to remember what has been validated and what has not, and then act accordingly at the end of the function. If it's OK to stop after finding out that the first one doesn't match, then that's fine. \$\endgroup\$ – Quentin Pradet Mar 8 '12 at 13:20
  • \$\begingroup\$ Hey @Cygal you are right that if first one is wrong then others wont be evaluated. So tell me how can i fix that is it good to use a variable with value 0 if matched, non-zero if not matched and in last redirects the user checking the variables value? \$\endgroup\$ – Shayan Husaini Mar 9 '12 at 6:40
  • \$\begingroup\$ Yes, that would work. I'll edit my answer to explain it a bit more. \$\endgroup\$ – Quentin Pradet Mar 9 '12 at 7:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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