[Coding style] Tests for false

Topic: [Coding style] Tests for false (Read 2277 times) previous topic - next topic

0 Members and 1 Guest are viewing this topic.

[Coding style] Tests for false

July 06, 2016, 08:44:03 am

I'd have a proposal on the coding style: in several conditionals we do test for negative with the ! operator, I don't know you, but I find it "difficult" to read, because it actually is small and is easy not to notice, e.g.

Code: [Select]

if (!$cache->checkLevel(2) || !$cache->getVar($user_settings, 'user_settings-' . $id_member, 60))

it's a long line and the two ! are somehow hidden.

So, I was thinking to propose, at least when function calls are involved, to use the strict "=== false" notation, so that it is a little easier to spot the negative comparison:

Code: [Select]

if ($cache->checkLevel(2) === false || $cache->getVar($user_settings, 'user_settings-' . $id_member, 60) === false)

In my opinion, this has two advantages: the immediate one is, as explained, to make it easier to read the code, the indirect one is that it forces us to ensure functions that may fail to return a false instead of anything from empty string to null.

Of course not something that I would change overnight, I'd start with new functions and then cleanup all the way back.

I'm unsure on how to deal with the various

Code: [Select]

!empty($something)

Re: [Coding style] Tests for false

Reply #1 – July 06, 2016, 07:45:39 pm

Why restricting the code to certain fashion when you can make a comment / explanation for the code?

Re: [Coding style] Tests for false

Reply #2 – July 07, 2016, 03:58:12 am

For the same reason coding style rules/guidelines exist in the first place?
If anybody would pick their preferred style of coding, the syntax would be an utterly mess, with pieces of code using tabs and pieces using spaces for indentation, a mixture of K&R, Allman, maybe Whitesmiths (or any other indent style available, or some invented for the case), etc.
Any of these variants make the code more difficult to read, because any time you encounter some different style, you have to take a break (even only half a second) and switch mentally to that different style, then the time you get two styles mixed together you are doomed.

It's easier to read something like this:

Code: [Select]

	foreach ($data as $var => $val)
	{
		$type = 'string';

		if (in_array($var, $knownInts))
		{
			$type = 'int';
		}
		elseif (in_array($var, $knownFloats))
		{
			$type = 'float';
		}
		elseif ($var == 'birthdate')
		{
			$type = 'date';
		}

		// Doing an increment?
		if ($type == 'int' && ($val === '+' || $val === '-'))
		{
			$val = $var . ' ' . $val . ' 1';
			$type = 'raw';
		}

		// Ensure posts, personal_messages, and unread_messages don't overflow or underflow.
		if (in_array($var, array('posts', 'personal_messages', 'unread_messages')))
		{
			if (preg_match('~^' . $var . ' (\+ |- |\+ -)([\d]+)~', $val, $match))
			{
				if ($match[1] != '+ ')
				{
					$val = 'CASE WHEN ' . $var . ' <= ' . abs($match[2]) . ' THEN 0 ELSE ' . $val . ' END';
				}
				$type = 'raw';
			}
		}

		$setString .= ' ' . $var . ' = {' . $type . ':p_' . $var . '},';
		$parameters['p_' . $var] = $val;
	}

or:

Code: [Select]

	$setString = '';
	foreach ($data as $var => $val)
	{
		$type = 'string';

		if(in_array($var,$knownInts)){$type = 'int';}
		elseif (in_array($var, $knownFloats)){
			$type = 'float';
		} elseif ($var == 'birthdate')
			$type = 'date';

		// Doing an increment?
		if ($type == 'int' && ($val === '+' || $val === '-')) {
			$val = $var . ' ' . $val . ' 1';
			$type = 'raw';
		}

		// Ensure posts, personal_messages, and unread_messages don't overflow or underflow.
		if (in_array($var,
		array('posts', 'personal_messages', 'unread_messages'))){
			if (preg_match('~^' . $var . ' (\+ |- |\+ -)([\d]+)~', $val, $match))
			{
				if ($match[1] != '+ '){$val = 'CASE WHEN ' . $var . ' <= ' . abs($match[2]) . ' THEN 0 ELSE ' . $val . ' END';}
				$type = 'raw';
			}}

		$setString .= ' ' . $var . ' = {' . $type . ':p_' . $var . '},';
		$parameters['p_' . $var] = $val;
	}

You need coding style for the same reason you need a cultural background: easier communication.

Re: [Coding style] Tests for false

Reply #3 – July 07, 2016, 07:32:52 pm

I follow PSR-2.

Re: [Coding style] Tests for false

Reply #4 – July 08, 2016, 03:56:48 am

Yeah... I would as well if they didn't decide on the "4 spaces" rule for indenting instead of tabs.

Re: [Coding style] Tests for false

Reply #5 – July 11, 2016, 10:58:00 pm

I used to think that too. That was the hardest thing for me to change. But it grew on me.

The first few months I started following PSR-2, I kept indenting with tabs. Served as a good transitioning phase.

Re: [Coding style] Tests for false

Reply #6 – July 12, 2016, 03:43:56 am

Would be able to see the 4 spaces as 2 like I have set for tabs it wouldn't be such a great issue, but it's annoying to see that much space wasted for a decision made by someone else.
It sounds almost like python, but without the fact that it's part of the syntax.

That said, I find a bit silly make "rules" on that kind of visual aspect, to me a "let's be consistent within the project" would have been much nicer rule (and that includes where brackets are placed, and so on).
Programmers are not that stupid to be tricked by a 4 spaces instead of a tab, it's just annoying having an inconsistent project (and no, I don't consider dependencies part of the project, because nowadays, most of them are just few lines of JSON in composer.json) with 5 lines with tabs, a couple with spaces, then tabs, etc.
Of course IMHO.

Re: [Coding style] Tests for false

Reply #7 – July 13, 2016, 12:09:45 am

Working with some rancid code, this is a minute thing. I almost always use strict comparison, but the simple !$var is really nice sometimes.