Return to Answer

any rule can match.

Source Link

edited Mar 9, 2012 at 7:55

7.1k
1
25
44

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(given a name such as validate_demographics, and you). 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.

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, and 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?

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?

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.

Source Link

answered Mar 8, 2012 at 12:59

Quentin Pradet

7.1k
1
25
44

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, and 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?