ntemple / bracketpress

WordPress Plugin to manage 2013 NCAA Men's Basketball Tournament Bracket
4 stars 2 forks source link

Replace constant() with defined() #8

Open pippinsplugins opened 10 years ago

pippinsplugins commented 10 years ago

While I really don't think there's anything wrong with using the constant() function, it's a bit cleaner and more standard to use defined().

if (@constant('BRACKETPRESS_DEBUG')) {

becomes

if ( defined('BRACKETPRESS_DEBUG')) {
pippinsplugins commented 10 years ago

This is in the main plugin file and also here: https://github.com/ntemple/bracketpress/blob/master/bracketpress.php#L699

ntemple commented 10 years ago

The code is correct as written.

The intent of the code is to check if BRACKETPRESS_DEBUG is defined, and if it is defined, if it is truthy.

defined() only tells you if the constant is defined or not.

constant(), on the other hand, will tell you: a) if it's defined, or NULL it it is not defined b) what the value is if it is indeed defined (again, or NULL if undefined) (it will also throw a warning if not defined, which we suppress with the @ sign: http://www.php.net/manual/en/language.operators.errorcontrol.php )

See: http://us2.php.net/manual/en/function.constant.php

In the case it's defined but false, or in the case it's not defined at all, the debug code will not be run without printing errors or warnings (the @ suppresses any warnings).

The alternative would be the much more verbose:

if (defined('BRACKETPRESS_DEBUG') && BRACKETPRESS_DEBUG != false) { } which is still slightly incorrect because, if not defined, BRACKETPRES_DEBUG becomes a string, which evaluates to true and subtly incorrect.

Any specific reason you bring this up?

On Sun, Feb 9, 2014 at 8:28 PM, Pippin Williamson notifications@github.comwrote:

This is in the main plugin file and also here: https://github.com/ntemple/bracketpress/blob/master/bracketpress.php#L699

Reply to this email directly or view it on GitHubhttps://github.com/ntemple/bracketpress/issues/8#issuecomment-34600182 .

pippinsplugins commented 10 years ago

That makes sense. For me it was mostly for "standards" reasons, but I fully understand that's not always a good reason :)

Is there any reason that BRACKETPRESS_DEBUG would ever be defined as anything other than true? Just by accident?

ntemple commented 10 years ago

In a community project, I'd treat it as "user input".

TRUE, true, 'true', 1, YES, etc should all be ok, and no "valid" input should break the code. i.e., commenting / uncommenting the line (which breaks defined) or defining it as something unexpected. Esp. with debug type stuff, it's really person-dependent. If you're super concerned, we could add a note on the "accepted" way to enable / disable debugging in the README.md. As bad as the front-end view logic is (esp. the javascript) debug code is pretty low on the priority list.

On Mon, Feb 10, 2014 at 12:30 PM, Pippin Williamson < notifications@github.com> wrote:

That makes sense. For me it was mostly for "standards" reasons, but I fully understand that's not always a good reason :)

Is there any reason that BRACKETPRESS_DEBUG would ever be defined as anything other than true? Just by accident?

Reply to this email directly or view it on GitHubhttps://github.com/ntemple/bracketpress/issues/8#issuecomment-34678543 .

pippinsplugins commented 10 years ago

Ha I wasn't "concerned" about it :)

Very good points.

On Mon, Feb 10, 2014 at 10:03 PM, Nick Temple notifications@github.comwrote:

In a community project, I'd treat it as "user input".

TRUE, true, 'true', 1, YES, etc should all be ok, and no "valid" input should break the code. i.e., commenting / uncommenting the line (which breaks defined) or defining it as something unexpected. Esp. with debug type stuff, it's really person-dependent. If you're super concerned, we could add a note on the "accepted" way to enable / disable debugging in the README.md. As bad as the front-end view logic is (esp. the javascript) debug code is pretty low on the priority list.

On Mon, Feb 10, 2014 at 12:30 PM, Pippin Williamson < notifications@github.com> wrote:

That makes sense. For me it was mostly for "standards" reasons, but I fully understand that's not always a good reason :)

Is there any reason that BRACKETPRESS_DEBUG would ever be defined as anything other than true? Just by accident?

Reply to this email directly or view it on GitHub< https://github.com/ntemple/bracketpress/issues/8#issuecomment-34678543>

.

Reply to this email directly or view it on GitHubhttps://github.com/ntemple/bracketpress/issues/8#issuecomment-34724738 .