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

code cleanup #5813

Merged
merged 2 commits into from Jul 15, 2014
Merged

code cleanup #5813

merged 2 commits into from Jul 15, 2014

Conversation

craue
Copy link
Contributor

@craue craue commented Jul 10, 2014

The 1st commit can be considered to be a bugfix because the wrong variable was used there, at least with my understanding of the code.

The 2nd 1st commit removes unused variables while trying not to break stuff, as the code seems to be pretty fragile. There are still plenty of unused variables when calling methods in the manner of naming arguments like for self::json_decode($value, $assoc = true) where $assoc is in fact useless. I could remove them all as well.

The 3rd 2nd commit fixes the order of keywords in method signatures:
static (public|protected|private) function(public|protected|private) static function.

Let me know what you think.

@craue
Copy link
Contributor Author

craue commented Jul 14, 2014

I'll rebase the branch in case you want to merge it. 😏

@@ -134,7 +134,7 @@ protected function applyGenericFilters($datatable)
}

try {
$value = Common::getRequestVar($name, $defaultValue, $type, $this->request);
$value = Common::getRequestVar($varName, $defaultValue, $type, $this->request);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could put this in a different pull request since it looks like it could fix a bug and is not just code cleanup?

@mattab
Copy link
Member

mattab commented Jul 15, 2014

I'll rebase the branch in case you want to merge it. 😏

Sounds good, we'll definitely merge it in!

@craue craue mentioned this pull request Jul 15, 2014
@craue
Copy link
Contributor Author

craue commented Jul 15, 2014

Outsourced the bugfix into #5835. Remainder rebased.

The changes only apply to the core folder. Should other folders (plugins, tests, but probably not libs) also be considered?
How to deal with the other unused variables I mentioned initially?

@mattab
Copy link
Member

mattab commented Jul 15, 2014

There are still plenty of unused variables when calling methods in the manner of naming arguments like for self::json_decode($value, $assoc = true) where $assoc is in fact useless. I could remove them all as well.

sometimes we put the variable even though it's the default value, because it may help readability. So I wouldn't change that.

The changes only apply to the core folder. Should other folders (plugins, tests, but probably not libs) also be considered?

+1, plugins/ and tests/ are also part of the core platform.

mattab pushed a commit that referenced this pull request Jul 15, 2014
@mattab mattab merged commit 4c59080 into matomo-org:master Jul 15, 2014
@mattab
Copy link
Member

mattab commented Jul 15, 2014

Thanks for pull request!

@craue
Copy link
Contributor Author

craue commented Jul 15, 2014

There are still plenty of unused variables when calling methods in the manner of naming arguments like for self::json_decode($value, $assoc = true) where $assoc is in fact useless. I could remove them all as well.

sometimes we put the variable even though it's the default value, because it may help readability. So I wouldn't change that.

I don't agree with that. Those variables clutter the code, making method calls much longer than needed and thus harder to read. A proper IDE will show the method signature quickly when needed, but there are numerous warnings about unused variables because of this, which makes it hard to spot those which are in fact typos (or bugs).

@craue craue mentioned this pull request Jul 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants