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

Integration with beberlei/assert library #8

Merged
merged 5 commits into from
Mar 20, 2016
Merged

Conversation

pdaw
Copy link
Member

@pdaw pdaw commented Mar 20, 2016

No description provided.

@@ -32,6 +32,11 @@ class ContractCheckerAspect implements Aspect
private $reader = null;

/**
* @var MethodInvocation
*/
private $invocation;
Copy link
Member

Choose a reason for hiding this comment

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

Could you please describe, why do you need to store an invocation object in the private property?

Copy link
Member Author

Choose a reason for hiding this comment

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

I created private property to have access to MethodInvocation instance in ContractCheckerAspect::170 line. I don't want to create new parameter in isContractSatisfied method for this (it would be 5th parameter).

Why do we throw exception here? If we return false instead of raising exception, then we lose previous exception (for example - assertion exception) details.

Copy link
Member

Choose a reason for hiding this comment

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

I think that private property here can be incorrect for recursive calls of methods, etc, this adds some extra overhead for assigning the value to the property for each method call, maybe it will be better to rewrite code as following:

  • rename isContractSatisfied to ensureContractSatisfied that will throw simple DomainException if check was failed (bool false)
  • change all ifs in advice's body to try...catch construction that rethrow a ContractViolation exception
  • all exceptions now will be in one place, no need for property and extra argument

Copy link
Member Author

Choose a reason for hiding this comment

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

I like it, ok

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, after that change I'll merge this PR 👍

@lisachenko lisachenko added this to the 0.3.0 milestone Mar 20, 2016
lisachenko added a commit that referenced this pull request Mar 20, 2016
Integration with beberlei/assert library, resolves #3
@lisachenko lisachenko merged commit 83ac96f into php-deal:master Mar 20, 2016
@lisachenko
Copy link
Member

Merged, thanks!

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.

2 participants