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

Collection::transpose method #8957

Merged
merged 14 commits into from Jun 17, 2016

Conversation

Projects
None yet
8 participants
@dilab
Copy link
Contributor

commented Jun 9, 2016

This is my implementation of Collection::transpose method.

As discussed in #8955

@markstory markstory added this to the 3.3.0 milestone Jun 9, 2016

@markstory

This comment has been minimized.

Copy link
Member

commented Jun 9, 2016

There are a few PHCS errors.

@@ -30,7 +30,7 @@
"ext-openssl": "To use Security::encrypt() or have secure CSRF token generation."
},
"require-dev": {
"phpunit/phpunit": "*",
"phpunit/phpunit": "4.8",

This comment has been minimized.

Copy link
@markstory

markstory Jun 9, 2016

Member

Please don't do this.

// when cakephp require PHP >= 5.6
// return (new Collection(array_map(function (...$line) {
// return $line;
// }, ...$this->toArray())));

This comment has been minimized.

Copy link
@markstory

markstory Jun 9, 2016

Member

This should be removed.

*/
public function transpose()
{
$result = array();

This comment has been minimized.

Copy link
@markstory

markstory Jun 9, 2016

Member

Please use [] style arrays.

*
* @copyright Copyright (c) Cake Software Foundation, Inc. (http://cakefoundation.org)
* @link http://cakephp.org CakePHP(tm) Project
* @since 3.0.5

This comment has been minimized.

Copy link
@markstory

markstory Jun 9, 2016

Member

Version number should be 3.3.0

use MultipleIterator;
class TransposeIterator extends MultipleIterator

This comment has been minimized.

Copy link
@markstory

markstory Jun 9, 2016

Member

If we're not doing anything special in the subclass we shouldn't have a subclass.

* User: xu
* Date: 8/6/16
* Time: 7:31 PM
*/

This comment has been minimized.

Copy link
@markstory

markstory Jun 9, 2016

Member

Can you use the standard project file headers.

['Product A', '200', '100', '50'],
['Product B', '300', '200', '100'],
['Product C', '400', '300', '200'],
]);

This comment has been minimized.

Copy link
@markstory

markstory Jun 9, 2016

Member

What happens when the arrays are of uneven lengths?

@@ -945,4 +945,35 @@ public function isEmpty();
* @return \Iterator
*/
public function unwrap();

This comment has been minimized.

Copy link
@jippi

jippi Jun 9, 2016

Member

double lines

* @license http://www.opensource.org/licenses/mit-license.php MIT License
*/
namespace Cake\Test\TestCase\Collection\Iterator;

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Jun 10, 2016

Contributor

There must be one blank line after the namespace declaration

* // ]
*
* ```
*

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Jun 10, 2016

Contributor

Whitespace found at end of line

$expected = [
['Products', 'Product A', 'Product B', 'Product C'],
['2012', '200', '300', '400'],
];

This comment has been minimized.

Copy link
@ndm2

ndm2 Jun 10, 2016

Contributor

Is silently dropping items a good idea? I could see this easily leading to problems/making the method unfit for various purposes. Maybe it would be better to throw an exception instead, or add an option for filling up the missing items with for example null. At the very least this truncating behavior would need to be documented clearly.

This comment has been minimized.

Copy link
@TheFRedFox

TheFRedFox Jun 10, 2016

Contributor

I was also thinking about that. Without any documentation I would suspect that it would return null for not set cells. So:

['Products', 'Product A', 'Product B', 'Product C'],
['2012', '200', '300', '400'],
['2013', '100', null, '300'],
['2014', '50', null, null],

However in the discussion it was shown as an analogue to the ruby one .. ruby is raising an IndexError when the length of the subarrays are different. [1]

[1] http://ruby-doc.org/core-2.2.0/Array.html#method-i-transpose

{
$length = $this->map(function ($row) {
return count($row);
});

This comment has been minimized.

Copy link
@lorenzo

lorenzo Jun 10, 2016

Member

You could just read the count of the first row. That should be enough, if you get a row with more or less elements that expected, throw an exception

@dilab

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2016

@markstory I realise we have to pass both php 5.9 and php 7 for the test cases. what is your strategy you use to run test cases in both PHP version locally?

@dilab

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2016

I have implemented what you guys suggested. Throw exception if the array length is not even.

@markstory

This comment has been minimized.

Copy link
Member

commented Jun 11, 2016

@dilab I run the tests locally against PHP7, but keep in mind the limitations of PHP5.x 😄

if (count($row) != $length) {
throw new \LogicException('Child arrays do not have even length');
}
$result[] = array_column($this->toArray(), $column);

This comment has been minimized.

Copy link
@markstory

markstory Jun 11, 2016

Member

You're calling toArray() multiple times. You should probably keep a local variable with the original value. toArray() is not a cheap method to run.

@dilab

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2016

@markstory Updated using a local tmp variable to keep track of array values

*/
public function transpose()
{
$length = count($this->first());

This comment has been minimized.

Copy link
@markstory

markstory Jun 11, 2016

Member

This could be count(current($arrayValue)); as well.

This comment has been minimized.

Copy link
@dilab

dilab Jun 12, 2016

Author Contributor

Updated

@markstory markstory self-assigned this Jun 13, 2016

@markstory

This comment has been minimized.

Copy link
Member

commented Jun 14, 2016

What do you think @lorenzo ?

@ADmad

This comment has been minimized.

Copy link
Member

commented Jun 14, 2016

@dilab Can you please squash the commits into a single commit

*/
public function transpose()
{
$arrayValue = $this->toArray();

This comment has been minimized.

Copy link
@lorenzo

lorenzo Jun 14, 2016

Member

I think this should be toList instead of toArray. The reason is that using keys main considerably alter the result of this method to a ways that would be almost impossible to prevent by the user. Since we are dropping the keys in this method anyway, I think it is OK to use that.

This comment has been minimized.

Copy link
@dilab

dilab Jun 15, 2016

Author Contributor

Updated to toList() instead of toArray()

This comment has been minimized.

Copy link
@dilab

dilab Jun 15, 2016

Author Contributor

@ADmad I will do the squash when all the changes are done.

@markstory markstory merged commit 9114949 into cakephp:3.next Jun 17, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.

markstory added a commit that referenced this pull request Jun 17, 2016

dakota pushed a commit that referenced this pull request Nov 1, 2017

Add Collection::transpose method (#8957)
* Add transpose method to collection class
* Revert composer.json file
* Remove TransposeIterator subclass
* Remove double lines
* Use standard CakePHP file header
* Handle uneven length array
* Remove debug line
* Remove some space to please lint
* Throw LogicException if arrays do not share the same length
* Use a local variable to keep array value
* Use current() instead of first()
* Use toList() instead of toArray() to preserve all values

dakota pushed a commit that referenced this pull request Nov 1, 2017

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.