9 - Changes the way that we were currently encoding the parameters to…

[gabriel-felipe]

FIXES #9

The bug was caused by both the default parse_str function from PHP which automatically decodes the query string values and our old implementation of the encode() function. I've tweaked the implementation so we don't need PHP's parse_str and changed our encode() function to work as a blacklisting scheme.

I updated the README to clearly state the goals of this project and avoiding confusion.

And finally, I added an extra test case that would fail on Master due to the bug being fixed here.

[gabriel-felipe]

@azertys Can you help me review this? The bug being fixed here was the bug that caused our file URLs to break.

[enricodias]

The use of isset is discouraged, the function can be written to work without it.

The comparsion if ($queryString) hurts some patterns, it should be if ($queryString !== ''). But if this function is parsing query strings, if should be called only if a query string is present, if ($parsedUrl['query'] !== '') in the parent function.

The if ($k > 0) on each loop will increase the complexity unnecessarily, why not using implode() after the loop? Here is a quick (but untested) example:

$result = array();
$paramSkeleton = array(0 => '', 1 => '');

foreach ($params as $k => $param) {

    $paramParts = array_merge($paramSkeleton, explode('=', $param, 2));

    $result[encode($paramParts[0])] = encode($paramParts[1]);

}

$result = implode('&', $result);

if ($queryHasTrailingEqual !== false) $result .= '&';

[enricodias]

You can also use array_key_exists instead of merging arrays. This may be better if only 1 key needs to be checked.

[gabriel-felipe]

Thanks @enricodias could you elaborate a bit more on

1. Why is the use of isset is discouraged?
2. What patterns does if ($queryString) break? Did we decide to follow those patterns here? Maybe it's mentioned somewhere on the PSRs and I don't remember.

Sorry but I disagree with the optimizations you suggested, it looks like micro-optimizations to me that exchange a micro-performance improvement for worse readability. I prefer to let it as is with string concatenation because this won't ever be a performance bottleneck given the use case and it's way clearer to read. Also, the example you provided wouldn't work, so I don't see the point of rewriting it on a different way just for the sake of it.

---

@azertys Alin let me know your thoughts here about both the code changes and @enricodias 's suggestions.

---

@enricodias Thank you for all the help on this project, but I'd like to ask for us to respect the assigned reviewers of each pull request unless we see something critical that requires changes or the assigned reviewer let it stall for too long. I would also like to avoid nitpicking so we can keep the work here going with less friction.

[enricodias]

@gabriel-felipe

1. isset() checks if a variable was declared and it's not null. If we need to check if a variable was declared either we are doing something very clever, which as we know is an awful habit, or we are working with a completely procedural legacy system. In this case you used it to check if an array key exists and there is a specific function for that. The pattern isset($var)) ? $var : ''; is a practice widely used in poor code to suppress E_NOTICE errors.

2. Strict comparison is the best practice to prevent problems. if ($var) will return false if $var = '0'. For example, your code may fail in the url "http://domain.com/?0" by returning "http://domain.com/". Your intent was to check the content of the string, but you just checked if the the string is evaluated as true.

The changes I suggested are not micro optimizations at all (they are probably slower, in fact). They are not intended to speed up the execution, but to reduce the method's Cyclomatic Complexity, NPathComplexity and the C.R.A.P index. Those are metrics widely used to measure code quality.

Your code may be easier to read for you because you most likely learned to code in this way. In most code analysis tools this PR would either fail or introduce several issues, reducing the code quality of the project.

I know you assigned a reviewer, but this doesn't mean that others may not comment on the issue. Since this is a public repo (and without a contributing file with guidelines), it's normal expect opinions from others who contributed before. I thought that any contribution would be appreciated and I even took the time to write a code example. But ok, I'll stop commenting.

[azertys]

@gabriel-felipe Hey Gabriel, code seems to work good here. I couldn't break it.

@azertys Alin let me know your thoughts here about both the code changes and @enricodias 's suggestions.

Sure, suggestions are good but most do fall into the nitpicking category in my opinion. The if ($queryString) thing is a good one though because we want to avoid mixing types in that particular case.

@enricodias Hey Enrico, I think Gabriel was trying to say that we should look more for breaking changes, rather than trying to refactor such a small function with little gains. Comments and suggestion are always welcome!

[gabriel-felipe]

@enricodias Thanks for you explanation, it makes a lot of sense now. As Alin said, I was just trying to avoid friction and refactors with small gains, so I just wanted to define the culture of the project. Suggestions are always welcomed. You are also absolutely right that we don't have a contributing file and that should be fixed. I'll open an issue so we can decide the best practices to contribute to this project. Sorry if I couldn't express myself well, your suggestions have helped the code quality here a lot so far.

@azertys Thanks! That was my goal indeed.

---

Changes I made here:

• Changed isset for array_key_exists
• Avoided comparing strings as booleans
• Replaced string concatenation for implode

I didn't move the condition if ($queryString !== '') { to the parent function because if in the future this function is called somewhere else, it should know what to return anyway and avoid entering the into that if block unnecessary. Let me know your thoughts.

[enricodias]

Ok, I understand. Those changes may result in small gains now as the project itself is small, but they will payoff in the long term.

For now on I'll comment only breaking changes. When I see a code that can be improved I'll change it and explain in a separate PR.

[gabriel-felipe]

Thanks @enricodias

@azertys Feel free to merge this one if you think this is done.

[azertys]

@gabriel-felipe Seems done, but we should be careful with that array_key_exists, because it doesn't check for null values and the encode function requires a string to be passed as an argument. If you pass null, it will throw an exception. That said, I don't think explode will ever return a null value, so we should be safe but worth a double check on your side.

P.S. I think only you have push access to the repo.

[gabriel-felipe]

@azertys Thanks Alin! You are right, I also agree that we don't need to check that case since explode only returns false or array, and it only returns false if it's an empty string, but if it was an empty string it wouldn't get that far in the code. So we are fine. Merging this one back.

I gave permission to you and the whole EM team to admin this repo :)