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

Handle new _serialize === true functionality #11

Merged
merged 3 commits into from
Oct 29, 2015
Merged

Handle new _serialize === true functionality #11

merged 3 commits into from
Oct 29, 2015

Conversation

Zuluru
Copy link
Contributor

@Zuluru Zuluru commented Oct 28, 2015

Resolution for issue #10

}
$this->Controller->set('_serialize', $serializeKeys);
Copy link
Owner

Choose a reason for hiding this comment

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

It would be cleaner and easier to just return early

if ($this->Controller->viewVars['_serialize'] === true) {
     return; 
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to agree, but so many people seem obsessed with having only a single return point from functions. I'll make the change and submit a new PR?

Copy link
Owner

Choose a reason for hiding this comment

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

Having return early is one of the most useful code practices. I dont know any sane person who would argue against that.

Copy link
Owner

Choose a reason for hiding this comment

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

No need, you can just update this PR with a 2nd commit - by pushing to the same branch.

@dereuromark
Copy link
Owner

See, so much cleaner :) Beautiful, thx!
A test case would be nice, but aint necessary I guess.

@dereuromark
Copy link
Owner

PS: Tests are failing, you need an additional !empty($this->Controller->viewVars['_serialize']) check.

@Zuluru
Copy link
Contributor Author

Zuluru commented Oct 29, 2015

In the interests of reducing duplicate code, I'm tempted to put the new block inside the existing !empty condition. Seems the code could be less clear that way though. Got a preference?

@dereuromark
Copy link
Owner

return early still trumps it IMO as more levels of nesting is always worse.
But either way works.

dereuromark added a commit that referenced this pull request Oct 29, 2015
Handle new _serialize === true functionality
@dereuromark dereuromark merged commit db76b73 into dereuromark:master Oct 29, 2015
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