Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve readability of code #6343

Merged
merged 2 commits into from Sep 30, 2014

Conversation

Projects
None yet
2 participants
@tsteur
Copy link
Member

commented Sep 30, 2014

  • coding style fixes

  • some PHPStorm code inspection fixes

  • few method extractions

  • removed unused methods

  • whitespace fixes

    all as part of our code cleanup strategy and to make code better readable.

tsteur added some commits Sep 30, 2014

coding style fixes, some PHPStorm inspection fixes, improved readabil…
…ity of code, few refactorings, all as part of our code cleanup strategy
Merge branch 'master' into readabilityImprovements
Conflicts:
	core/Tracker/Db/Mysqli.php

tsteur added a commit that referenced this pull request Sep 30, 2014

@tsteur tsteur merged commit de5c066 into master Sep 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@tsteur tsteur deleted the readabilityImprovements branch Sep 30, 2014

return Db::fetchAll(self::getSqlAccessSite("access, t2.idsite"), $login);
$sql = self::getSqlAccessSite("access, t2.idsite");
return Db::fetchAll($sql, $login);

This comment has been minimized.

Copy link
@kylekatarnls

kylekatarnls Oct 1, 2014

Contributor

Really needed? This is only one braces pair embedded.

This comment has been minimized.

Copy link
@tsteur

tsteur Oct 2, 2014

Author Member

I don't see a huge advantage of having everything in one line. This way it is better readable and debuggable for me.

$siteTable = Common::prefixTable('site');
return "SELECT " . $select . " FROM " . $access . " as t1
JOIN " . $siteTable . " as t2 USING (idsite) WHERE login = ?";

This comment has been minimized.

Copy link
@kylekatarnls

kylekatarnls Oct 1, 2014

Contributor

Do you follow a rule? Because IMHO, or the query is considered complexed and there is a new line before each keyword (FROM, JOIN, WHERE) or is simple and all can be put in one line.

// Temporary archive
return Rules::getMinTimeProcessedForTemporaryArchive($this->params->getDateStart(), $this->params->getPeriod(), $this->params->getSegment(), $this->params->getSite());
return Rules::getMinTimeProcessedForTemporaryArchive($dateStart, $period, $segment, $site);

This comment has been minimized.

Copy link
@kylekatarnls

kylekatarnls Oct 1, 2014

Contributor

The variables names and getters are redundant. If you consider this line too big, maybe you can do this:

return Rules::getMinTimeProcessedForTemporaryArchive(
            $this->params->getDateStart(),
            $this->params->getPeriod(),
            $this->params->getSegment(),
            $this->params->getSite()
        );

This comment has been minimized.

Copy link
@tsteur

tsteur Oct 2, 2014

Author Member

In this case using variables is for me not only better readable but also better debuggable as you can directly set a breakpoint before going into the function for instance. Will reply to the others later, don't have much time right now

@@ -218,8 +219,19 @@ public function opCacheInvalidate($filepath)
@opcache_invalidate($filepath, $force = true);
}
if (function_exists('apc_delete_file')) {
apc_delete_file($filepath);
@apc_delete_file($filepath);

This comment has been minimized.

Copy link
@kylekatarnls

kylekatarnls Oct 1, 2014

Contributor

As we talk about here:
#6184
Put @ to hide error messages is a bad pratice, you should better handle errors you know to be able to happen and let others appear to debug them.

If you get an error because the file does not exist, do not hide the error but test if the file exists (with file_exists) before execute the function.

return stripslashes($value);
}
return $value;

This comment has been minimized.

Copy link
@kylekatarnls

kylekatarnls Oct 1, 2014

Contributor

I don't think 2 return statements increase the readabillity. A contrario. I suggest $value = stripslashes($value); instead if you do not want to keep the ternary syntax.

This comment has been minimized.

Copy link
@tsteur

tsteur Oct 3, 2014

Author Member

true, one return is better. BTW: as I mentioned it is also about better debugging here not only about readability.
Not using ternary syntax usually allows you to quicker read/understand the code as it is harder to parse (in my head) when having a look at the whole file etc (at least for me)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.