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

[Minor] Refactoring to avoid duplicate code #1233

Merged
merged 5 commits into from
Jan 4, 2015

Conversation

SofHad
Copy link
Contributor

@SofHad SofHad commented Dec 30, 2014

Very merry end of the year :)

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3466

We use Jira to track the state of pull requests and the versions they got
included in.

/**
* @var bool
*/
private $isChanged = false;
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong. Calling afterTransactionComplete multiple times does not work anymore, because you keep the state between calls

@stof
Copy link
Member

stof commented Dec 30, 2014

Your extracted private method looks weird with the different switches for the types. IMO, you should extract only the common logic (i.e. insert and update)

@SofHad
Copy link
Contributor Author

SofHad commented Dec 30, 2014

@stof, it is updated.
I extracted only the common logic (update, insert) into the private method handleCache.

@@ -121,4 +92,25 @@ public function update($entity)

$this->queuedCache['update'][] = $entity;
}

private function handleCache($entity, $isChanged)
Copy link
Member

Choose a reason for hiding this comment

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

I would call it updateCache

@SofHad
Copy link
Contributor Author

SofHad commented Dec 30, 2014

It is done @stof :)

@stof
Copy link
Member

stof commented Dec 30, 2014

looks good to me (but I'm not a merger on this repo)

@Ocramius Ocramius self-assigned this Dec 30, 2014
private function updateCache($entity, $isChanged)
{
$class = $this->class;
$className = ClassUtils::getClass($entity);
Copy link
Member

Choose a reason for hiding this comment

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

This check is actually useless: we can always ask the metadata factory directly instead

Copy link
Member

Choose a reason for hiding this comment

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

$class = $class = $this->metadataFactory->getMetadataFor(get_class($entity)) is enough in any case. ClassUtils::getClass() causes more internal method calls anyway.

@SofHad
Copy link
Contributor Author

SofHad commented Jan 4, 2015

@Ocramius
I removed this check.

@SofHad SofHad force-pushed the refactoring-duplicate-code branch from ab9fb61 to eb22db0 Compare January 4, 2015 20:02
@Ocramius
Copy link
Member

Ocramius commented Jan 4, 2015

Thanks! Merging :-)

Ocramius added a commit that referenced this pull request Jan 4, 2015
[Minor] Refactoring to avoid duplicate code
@Ocramius Ocramius merged commit 6448627 into doctrine:master Jan 4, 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.

4 participants