Skip to content

Conversation

@oliverklee
Copy link
Collaborator

Part of #756

@coveralls
Copy link

coveralls commented Feb 24, 2025

Coverage Status

coverage: 53.778%. remained the same
when pulling ed47f1e on cleanup/hungarian/start
into 6f1fc2c on main.

Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

I think $offset would be better throughout as a variable name.

* @param int $length
*/
private function substr($iStart, $length): string
private function substr($start, $length): string
Copy link
Collaborator

Choose a reason for hiding this comment

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

The equivalant PHP function has this parameter as $offset, which I think is better.

for ($iStartPosition = 0; $iStartPosition < $iStackLength; ++$iStartPosition) {
if ($iStartPosition === ($iStackLength - 1) || $sDelimiter !== $aStack[$iStartPosition + 1]) {
$aNewStack[] = $aStack[$iStartPosition];
for ($startPosition = 0; $startPosition < $iStackLength; ++$startPosition) {
Copy link
Collaborator

@JakeQZ JakeQZ Feb 24, 2025

Choose a reason for hiding this comment

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

Perhaps $offset would be a better name here too - particularly as it is changed during loop iteration to lose its original "start" meaning.

@oliverklee oliverklee force-pushed the cleanup/hungarian/start branch from 97996b5 to ed47f1e Compare February 24, 2025 11:06
@oliverklee oliverklee requested a review from JakeQZ February 24, 2025 11:06
@JakeQZ JakeQZ merged commit 7e812c8 into main Feb 24, 2025
21 checks passed
@JakeQZ JakeQZ deleted the cleanup/hungarian/start branch February 24, 2025 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants