TeamController - Code Metrics - Inspection of "Add a test for fun matches" - allejo/bzion - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Push — fm-support ( 0de470...22a3ee )

by Konstantinos

created 2016-08-28 22:27 UTC

TeamController B

↳ Parent: Project

Complexity

Total Complexity

Size/Duplication

Total Lines	203
Duplicated Lines	17.24 %

Coupling/Cohesion

Components	2
Dependencies	16

Test Coverage

Coverage

92%

Importance

Changes	5
Bugs	0	Features	3

Metric	Value
wmc	25
c	5
b	0
f	3
lcom	2
cbo	16
dl	35
loc	203
ccs	46
cts	50
cp	0.92
rs	8.4614

13 Methods

Rating	Name	Duplication	Size	Complexity
A	showAction()	0	16	1
A	listAction()	0	15	1
A	createAction()	0	4	1
A	deleteAction()	0	9	1
A	joinAction()	17	17	3
A	kickAction()	0	21	3
A	abandonAction()	18	18	3
A	assignLeaderAction()	0	20	3
A	newLeader()	0	4	1
A	validateNew()	0	11	2
A	canCreate()	0	8	2
A	assertCanEdit()	0	6	2
A	editAction()	0	17	2

How to fix Duplicated Code

<?php

use BZIon\Event as Event;
use BZIon\Event\Events;
use Symfony\Component\Form\FormError;
use Symfony\Component\HttpFoundation\RedirectResponse;

class TeamController extends CRUDController
{
    /**
     * The new leader if we're changing them
     * @var null|Player
     */
    private $newLeader = null;

    public function showAction(Team $team)
    {
        $matches = $this->getQueryBuilder('Match')
abstract class User
{
    /** @return string */
    abstract public function getPassword();
}

class MyUser extends User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}
            ->with($team)
            ->getSummary($team);

        $wins = $this->getQueryBuilder('Match')
abstract class User
{
    /** @return string */
    abstract public function getPassword();
}

class MyUser extends User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}
            ->with($team, "win")
            ->getSummary($team);

        return array(
            "matches" => $matches,
            "wins"    => $wins,
            "team"    => $team
        );
    }

    public function listAction()
    {
        Team::$cachedMatches = $this->getQueryBuilder('Match')
            ->where('time')->isAfter(TimeDate::from('45 days ago'))
            ->active()
            ->getModels($fast = true);

        $teams = $this->getQueryBuilder()
            ->sortBy('elo')->reverse()
            ->getModels($fast = true);

        return array(
            "teams" => $teams
        );
    }

    public function createAction(Player $me)
    {
        return $this->create($me);
    }

    public function editAction(Player $me, Team $team)
    {
        // TODO: Generating this response is unnecessary
        $response = $this->edit($team, $me, "team");

        if ($this->newLeader) {
            // Redirect to a confirmation form if we are assigning a new leader
            $url = Service::getGenerator()->generate('team_assign_leader', array(
                'team'   => $team->getAlias(),
                'player' => $this->newLeader->getAlias()
            ));

            return new RedirectResponse($url);
        }

        return $response;
    }

    public function deleteAction(Player $me, Team $team)
    {
        $members = $team->getMembers();

        return $this->delete($team, $me, function () use ($team, $me, $members) {
            $event = new Event\TeamDeleteEvent($team, $me, $members);
            Service::getDispatcher()->dispatch(Events::TEAM_DELETE, $event);
        });
    }

    public function joinAction(Team $team, Player $me)

    {
        $this->requireLogin();
        if (!$me->isTeamless()) {
            throw new ForbiddenException("You are already a member of a team");
        } elseif ($team->getStatus() != 'open') {
            throw new ForbiddenException("This team is not accepting new members without an invitation");
        }

        return $this->showConfirmationForm(function () use ($team, $me) {
            $team->addMember($me->getId());
            Service::getDispatcher()->dispatch(Events::TEAM_JOIN,  new Event\TeamJoinEvent($team, $me));

            return new RedirectResponse($team->getUrl());
        },  "Are you sure you want to join {$team->getEscapedName()}?",
            "You are now a member of {$team->getName()}");
    }

    public function kickAction(Team $team, Player $player, Player $me)
    {
        $this->assertCanEdit($me, $team, "You are not allowed to kick a player off that team!");

        if ($team->getLeader()->isSameAs($player)) {
            throw new ForbiddenException("You can't kick the leader off their team.");
        }

        if (!$team->isMember($player->getId())) {
            throw new ForbiddenException("The specified player is not a member of that team.");
        }

        return $this->showConfirmationForm(function () use ($me, $team, $player) {
            $team->removeMember($player->getId());
            $event = new Event\TeamKickEvent($team, $player, $me);
            Service::getDispatcher()->dispatch(Events::TEAM_KICK, $event);

            return new RedirectResponse($team->getUrl());
        },  "Are you sure you want to kick {$player->getEscapedUsername()} from {$team->getEscapedName()}?",
            "Player {$player->getUsername()} has been kicked from {$team->getName()}", "Kick");
    }

    public function abandonAction(Team $team, Player $me)

    {
        if (!$team->isMember($me->getId())) {
            throw new ForbiddenException("You are not a member of that team!");
        }

        if ($team->getLeader()->isSameAs($me)) {
            throw new ForbiddenException("You can't abandon the team you are leading.");
        }

        return $this->showConfirmationForm(function () use ($team, $me) {
            $team->removeMember($me->getId());
            Service::getDispatcher()->dispatch(Events::TEAM_ABANDON, new Event\TeamAbandonEvent($team, $me));

            return new RedirectResponse($team->getUrl());
        },  "Are you sure you want to abandon {$team->getEscapedName()}?",
            "You have left {$team->getName()}", "Abandon");
    }

    public function assignLeaderAction(Team $team, Player $me, Player $player)
    {
        $this->assertCanEdit($me, $team, "You are not allowed to change the leader of this team.");

        if (!$team->isMember($player->getId())) {
            throw new ForbiddenException("The specified player is not a member of {$team->getName()}");
        } elseif ($team->getLeader()->isSameAs($player)) {
            throw new ForbiddenException("{$player->getUsername()} is already the leader of {$team->getName()}");
        }

        return $this->showConfirmationForm(function () use ($player, $team) {
            $event = new Event\TeamLeaderChangeEvent($team, $player, $team->getLeader());
            $team->setLeader($player->getId());
            Service::getDispatcher()->dispatch(Events::TEAM_LEADER_CHANGE, $event);

            return new RedirectResponse($team->getUrl());
        }, "Are you sure you want to transfer the leadership of the team to <strong>{$player->getEscapedUsername()}</strong>?",
        "{$player->getUsername()} is now leading {$team->getName()}",
        "Appoint leadership");
    }

    /**
     * Show a confirmation form to confirm that the user wants to assign a new
     * leader to a team
     *
     * @param Player $leader The new leader
     */
    public function newLeader($leader)
    {
        $this->newLeader = $leader;
    }

    protected function validateNew($form)
    {
        $name = $form->get('name');
        $team = Team::getFromName($name->getData());

        // The name for the team that the user gave us already exists
        // TODO: This takes deleted teams into account, do we want that?
        if ($team->isValid()) {
            $name->addError(new FormError("A team called {$team->getName()} already exists"));
        }
    }

    protected function canCreate($player)
    {
        if ($player->getTeam()->isValid()) {
            throw new ForbiddenException("You need to abandon your current team before you can create a new one");
        }

        return parent::canCreate($player);
    }

    /**
     * Make sure that a player can edit a team
     *
     * Throws an exception if a player is not an admin or the leader of a team
     * @param  Player        $player  The player to test
     * @param  Team          $team    The team
     * @param  string        $message The error message to show
     * @throws HTTPException
     * @return void
     */
    private function assertCanEdit(Player $player, Team $team, $message = "You are not allowed to edit that team")
    {
        if (!$player->canEdit($team)) {
            throw new ForbiddenException($message);
        }
    }
}


Push — fm-support ( 0de470...22a3ee )

TeamController B

Complexity

Size/Duplication

Coupling/Cohesion

Test Coverage

Importance

13 Methods

How to fix Duplicated Code

Duplicated Code

Available Fixes

Available Fixes

1			<?php
2
3			use BZIon\Event as Event;
4			use BZIon\Event\Events;
5			use Symfony\Component\Form\FormError;
6			use Symfony\Component\HttpFoundation\RedirectResponse;
7
8			class TeamController extends CRUDController
9			{
10			/**
11			* The new leader if we're changing them
12			* @var null\|Player
13			*/
14			private $newLeader = null;
15
16	1		public function showAction(Team $team)
17			{
18	1		$matches = $this->getQueryBuilder('Match')
			0 ignored issues – show Bug introduced 2016-02-21 12:28 UTC by Report Bug Copy Issue Report It seems like you code against a specific sub-type and not the parent class `QueryBuilder` as the method `with()` does only exist in the following sub-classes of `QueryBuilder`: `MatchQueryBuilder`. Maybe you want to `instanceof` check for one of these explicitly? Let’s take a look at an example: abstract class User { /** @return string / abstract public function getPassword(); } class MyUser extends User { public function getPassword() { // return something } public function getDisplayName() { // return some name. } } class AuthSystem { public function authenticate(User $user) { $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName())); // do something. } } In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different sub-classes of User which does not have a getDisplayName() method, the code will break. Available Fixes Change the type-hint for the parameter: class AuthSystem { public function authenticate(MyUser $user) { / ... / } } Add an additional type-check: class AuthSystem { public function authenticate(User $user) { if ($user instanceof MyUser) { $this->logger->info(/* ... /); } // or alternatively if ( ! $user instanceof MyUser) { throw new \LogicException( '$user must be an instance of MyUser, ' .'other instances are not supported.' ); } } } Note:* PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case. Add the method to the parent class: abstract class User { /** @return string / abstract public function getPassword(); /* @return string */ abstract public function getDisplayName(); } Loading history...
19	1		->with($team)
20	1		->getSummary($team);
21
22	1		$wins = $this->getQueryBuilder('Match')
			0 ignored issues – show Bug introduced 2016-02-21 12:28 UTC by Report Bug Copy Issue Report It seems like you code against a specific sub-type and not the parent class `QueryBuilder` as the method `with()` does only exist in the following sub-classes of `QueryBuilder`: `MatchQueryBuilder`. Maybe you want to `instanceof` check for one of these explicitly? Let’s take a look at an example: abstract class User { /** @return string / abstract public function getPassword(); } class MyUser extends User { public function getPassword() { // return something } public function getDisplayName() { // return some name. } } class AuthSystem { public function authenticate(User $user) { $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName())); // do something. } } In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different sub-classes of User which does not have a getDisplayName() method, the code will break. Available Fixes Change the type-hint for the parameter: class AuthSystem { public function authenticate(MyUser $user) { / ... / } } Add an additional type-check: class AuthSystem { public function authenticate(User $user) { if ($user instanceof MyUser) { $this->logger->info(/* ... /); } // or alternatively if ( ! $user instanceof MyUser) { throw new \LogicException( '$user must be an instance of MyUser, ' .'other instances are not supported.' ); } } } Note:* PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case. Add the method to the parent class: abstract class User { /** @return string / abstract public function getPassword(); /* @return string */ abstract public function getDisplayName(); } Loading history...
23	1		->with($team, "win")
24	1		->getSummary($team);
25
26			return array(
27	1		"matches" => $matches,
28	1		"wins" => $wins,
29	1		"team" => $team
30			);
31			}
32
33	1		public function listAction()
34			{
35	1		Team::$cachedMatches = $this->getQueryBuilder('Match')
36	1		->where('time')->isAfter(TimeDate::from('45 days ago'))
37	1		->active()
38	1		->getModels($fast = true);
39
40	1		$teams = $this->getQueryBuilder()
41	1		->sortBy('elo')->reverse()
42	1		->getModels($fast = true);
43
44			return array(
45	1		"teams" => $teams
46			);
47			}
48
49			public function createAction(Player $me)
50			{
51			return $this->create($me);
52			}
53
54			public function editAction(Player $me, Team $team)
55			{
56			// TODO: Generating this response is unnecessary
57			$response = $this->edit($team, $me, "team");
58
59			if ($this->newLeader) {
60			// Redirect to a confirmation form if we are assigning a new leader
61			$url = Service::getGenerator()->generate('team_assign_leader', array(
62			'team' => $team->getAlias(),
63			'player' => $this->newLeader->getAlias()
64			));
65
66			return new RedirectResponse($url);
67			}
68
69			return $response;
70			}
71
72	1		public function deleteAction(Player $me, Team $team)
73			{
74	1		$members = $team->getMembers();
75
76			return $this->delete($team, $me, function () use ($team, $me, $members) {
77	1		$event = new Event\TeamDeleteEvent($team, $me, $members);
78	1		Service::getDispatcher()->dispatch(Events::TEAM_DELETE, $event);
79	1		});
80			}
81
82		View Code Duplication	public function joinAction(Team $team, Player $me)
			0 ignored issues – show Duplication introduced 2015-12-18 00:04 UTC by Report Bug Copy Issue Report This method seems to be duplicated in your project. Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation. You can also find more detailed suggestions in the “Code” section of your repository. Loading history...
83			{
84			$this->requireLogin();
85			if (!$me->isTeamless()) {
86			throw new ForbiddenException("You are already a member of a team");
87			} elseif ($team->getStatus() != 'open') {
88			throw new ForbiddenException("This team is not accepting new members without an invitation");
89			}
90
91			return $this->showConfirmationForm(function () use ($team, $me) {
92			$team->addMember($me->getId());
93			Service::getDispatcher()->dispatch(Events::TEAM_JOIN, new Event\TeamJoinEvent($team, $me));
94
95			return new RedirectResponse($team->getUrl());
96			}, "Are you sure you want to join {$team->getEscapedName()}?",
97			"You are now a member of {$team->getName()}");
98			}
99
100	1		public function kickAction(Team $team, Player $player, Player $me)
101			{
102	1		$this->assertCanEdit($me, $team, "You are not allowed to kick a player off that team!");
103
104	1		if ($team->getLeader()->isSameAs($player)) {
105			throw new ForbiddenException("You can't kick the leader off their team.");
106			}
107
108	1		if (!$team->isMember($player->getId())) {
109			throw new ForbiddenException("The specified player is not a member of that team.");
110			}
111
112			return $this->showConfirmationForm(function () use ($me, $team, $player) {
113	1		$team->removeMember($player->getId());
114	1		$event = new Event\TeamKickEvent($team, $player, $me);
115	1		Service::getDispatcher()->dispatch(Events::TEAM_KICK, $event);
116
117	1		return new RedirectResponse($team->getUrl());
118	1		}, "Are you sure you want to kick {$player->getEscapedUsername()} from {$team->getEscapedName()}?",
119	1		"Player {$player->getUsername()} has been kicked from {$team->getName()}", "Kick");
120			}
121
122	1	View Code Duplication	public function abandonAction(Team $team, Player $me)
			0 ignored issues – show Duplication introduced 2015-12-18 00:04 UTC by Report Bug Copy Issue Report This method seems to be duplicated in your project. Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation. You can also find more detailed suggestions in the “Code” section of your repository. Loading history...
123			{
124	1		if (!$team->isMember($me->getId())) {
125			throw new ForbiddenException("You are not a member of that team!");
126			}
127
128	1		if ($team->getLeader()->isSameAs($me)) {
129	1		throw new ForbiddenException("You can't abandon the team you are leading.");
130			}
131
132			return $this->showConfirmationForm(function () use ($team, $me) {
133	1		$team->removeMember($me->getId());
134	1		Service::getDispatcher()->dispatch(Events::TEAM_ABANDON, new Event\TeamAbandonEvent($team, $me));
135
136	1		return new RedirectResponse($team->getUrl());
137	1		}, "Are you sure you want to abandon {$team->getEscapedName()}?",
138	1		"You have left {$team->getName()}", "Abandon");
139			}
140
141			public function assignLeaderAction(Team $team, Player $me, Player $player)
142			{
143			$this->assertCanEdit($me, $team, "You are not allowed to change the leader of this team.");
144
145			if (!$team->isMember($player->getId())) {
146			throw new ForbiddenException("The specified player is not a member of {$team->getName()}");
147			} elseif ($team->getLeader()->isSameAs($player)) {
148			throw new ForbiddenException("{$player->getUsername()} is already the leader of {$team->getName()}");
149			}
150
151			return $this->showConfirmationForm(function () use ($player, $team) {
152			$event = new Event\TeamLeaderChangeEvent($team, $player, $team->getLeader());
153			$team->setLeader($player->getId());
154			Service::getDispatcher()->dispatch(Events::TEAM_LEADER_CHANGE, $event);
155
156			return new RedirectResponse($team->getUrl());
157			}, "Are you sure you want to transfer the leadership of the team to <strong>{$player->getEscapedUsername()}</strong>?",
158			"{$player->getUsername()} is now leading {$team->getName()}",
159			"Appoint leadership");
160			}
161
162			/**
163			* Show a confirmation form to confirm that the user wants to assign a new
164			* leader to a team
165			*
166			* @param Player $leader The new leader
167			*/
168			public function newLeader($leader)
169			{
170			$this->newLeader = $leader;
171			}
172
173			protected function validateNew($form)
174			{
175			$name = $form->get('name');
176			$team = Team::getFromName($name->getData());
177
178			// The name for the team that the user gave us already exists
179			// TODO: This takes deleted teams into account, do we want that?
180			if ($team->isValid()) {
181			$name->addError(new FormError("A team called {$team->getName()} already exists"));
182			}
183			}
184
185			protected function canCreate($player)
186			{
187			if ($player->getTeam()->isValid()) {
188			throw new ForbiddenException("You need to abandon your current team before you can create a new one");
189			}
190
191			return parent::canCreate($player);
192			}
193
194			/**
195			* Make sure that a player can edit a team
196			*
197			* Throws an exception if a player is not an admin or the leader of a team
198			* @param Player $player The player to test
199			* @param Team $team The team
200			* @param string $message The error message to show
201			* @throws HTTPException
202			* @return void
203			*/
204	1		private function assertCanEdit(Player $player, Team $team, $message = "You are not allowed to edit that team")
205			{
206	1		if (!$player->canEdit($team)) {
207			throw new ForbiddenException($message);
208			}
209	1		}
210			}
211

allejo / bzion

Push — fm-support ( 0de470...22a3ee )

TeamController B

Complexity

Size/Duplication

Coupling/Cohesion

Test Coverage

Importance

13 Methods

How to fix Duplicated Code

Duplicated Code

Available Fixes

Available Fixes

Duplication Side-by-Side

Filter issues like