Completed
Push — feature/player-elo ( 35076f...8ea10f )
by Vladimir
10:32
created

MatchController   C

Complexity

Total Complexity 46

Size/Duplication

Total Lines 294
Duplicated Lines 18.71 %

Coupling/Cohesion

Components 2
Dependencies 17

Test Coverage

Coverage 26.43%

Importance

Changes 0
Metric Value
wmc 46
lcom 2
cbo 17
dl 55
loc 294
ccs 37
cts 140
cp 0.2643
rs 6.16
c 0
b 0
f 0

11 Methods

Rating   Name   Duplication   Size   Complexity  
A showAction() 0 4 1
B recalculateAction() 0 33 4
A log() 0 6 1
B listAction() 0 34 4
B createAction() 16 16 5
A editAction() 16 16 3
B recalculate() 8 58 5
A getMessages() 0 15 4
A deleteAction() 15 15 4
D validate() 0 40 10
B validateEdit() 0 10 5

How to fix   Duplicated Code    Complexity   

Duplicated Code

Duplicate code is one of the most pungent code smells. A rule that is often used is to re-structure code once it is duplicated in three or more places.

Common duplication problems, and corresponding solutions are:

Complex Class

 Tip:   Before tackling complexity, make sure that you eliminate any duplication first. This often can reduce the size of classes significantly.

Complex classes like MatchController often do a lot of different things. To break such a class down, we need to identify a cohesive component within that class. A common approach to find such a component is to look for fields/methods that share the same prefixes, or suffixes. You can also have a look at the cohesion graph to spot any un-connected, or weakly-connected components.

Once you have determined the fields that belong together, you can apply the Extract Class refactoring. If the component makes sense as a sub-class, Extract Subclass is also a candidate, and is often faster.

While breaking up the class, it is a good idea to analyze how other classes use MatchController, and based on these observations, apply Extract Interface, too.

1
<?php
2
3
use Symfony\Component\Form\FormError;
4
use Symfony\Component\HttpFoundation\RedirectResponse;
5
use Symfony\Component\HttpFoundation\Request;
6
use Symfony\Component\HttpFoundation\StreamedResponse;
7
8
class MatchController extends CRUDController
9
{
10
    /**
11
     * Whether the last edited match has had its ELO changed, requiring an ELO
12
     * recalculation
13
     *
14
     * This is useful so that a confirmation form is shown, asking the user if
15
     * they want to recalculate ELOs
16
     *
17
     * @var bool
18
     */
19
    public $recalculateNeeded = false;
20
21
    public function listAction(Request $request, Player $me, Team $team = null, Player $player = null, $type = null)
22
    {
23
        $qb = $this->getQueryBuilder();
24
25
        $currentPage = $request->query->get('page', 1);
26
27
        if ($player) {
28
            $team = $player;
29
        }
30
31
        $query = $qb->sortBy('time')->reverse()
0 ignored issues
show
Bug introduced by
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

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. 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.
  1. Add the method to the parent class:

    abstract class User
    {
        /** @return string */
        abstract public function getPassword();
    
        /** @return string */
        abstract public function getDisplayName();
    }
    
Loading history...
32
               ->with($team, $type)
33
               ->limit(50)->fromPage($currentPage);
34
35
        $matchType = $request->query->get('type', 'all');
36
37
        if (in_array($matchType, array(Match::FUN, Match::OFFICIAL, Match::SPECIAL))) {
38
            $query->where('type')->is($matchType);
39
        }
40
41
        $matches = $query->getModels($fast = true);
42
43
        foreach ($matches as $match) {
44
            // Don't show wrong labels for matches
45
            $match->getOriginalTimestamp()->setTimezone($me->getTimezone());
46
        }
47
48
        return array(
49
            "matches"     => $matches,
50
            "team"        => $team,
51
            "currentPage" => $currentPage,
52
            "totalPages"  => $qb->countPages()
53
        );
54
    }
55
56 1
    public function showAction(Match $match)
57
    {
58 1
        return array("match" => $match);
59
    }
60
61 1 View Code Duplication
    public function createAction(Player $me)
0 ignored issues
show
Duplication introduced by
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...
62
    {
63
        return $this->create($me, function (Match $match) use ($me) {
64 1
            if ($me->canEdit($match)
65 1
                && $match->isOfficial()
66 1
                && (!$match->getTeamA()->isLastMatch($match)
67 1
                || !$match->getTeamB()->isLastMatch($match))
68
            ) {
69
                $url = Service::getGenerator()->generate('match_recalculate', array(
70
                    'match' => $match->getId(),
71
                ));
72
73
                return new RedirectResponse($url);
74
            }
75 1
        });
76
    }
77
78 View Code Duplication
    public function deleteAction(Player $me, Match $match)
0 ignored issues
show
Duplication introduced by
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...
79
    {
80
        return $this->delete($match, $me, function () use ($match, $me) {
81
            if ($match->getTeamA()->isLastMatch($match)
82
                && $match->getTeamB()->isLastMatch($match)) {
83
                $match->resetELOs();
84
            } elseif ($me->canEdit($match)) {
85
                $url = Service::getGenerator()->generate('match_recalculate', array(
86
                    'match' => $match->getId(),
87
                ));
88
89
                return new RedirectResponse($url);
90
            }
91
        });
92
    }
93
94 View Code Duplication
    public function editAction(Player $me, Match $match)
0 ignored issues
show
Duplication introduced by
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...
95
    {
96
        // TODO: Generating this response is unnecessary
97
        $response = $this->edit($match, $me, "match");
98
99
        if ($this->recalculateNeeded && $match->isOfficial()) {
100
            // Redirect to a confirmation form if we are assigning a new leader
101
            $url = Service::getGenerator()->generate('match_recalculate', array(
102
                'match' => $match->getId(),
103
            ));
104
105
            return new RedirectResponse($url);
106
        }
107
108
        return $response;
109
    }
110
111
    public function recalculateAction(Player $me, $match)
112
    {
113
        $match = Match::get($match); // get a match even if it's deleted
114
115
        if (!$me->canEdit($match)) {
116
            throw new ForbiddenException("You are not allowed to edit that match.");
117
        }
118
119
        if (!$match->isOfficial()) {
120
            throw new BadRequestException("You can't recalculate ELO history for a special match.");
121
        }
122
123
        return $this->showConfirmationForm(function () use ($match) {
124
            $response = new StreamedResponse();
125
            $response->headers->set('Content-Type', 'text/plain');
126
            $response->setCallback(function () use ($match) {
127
                $this->recalculate($match);
128
            });
129
            $response->send();
130
        }, "Do you want to recalculate ELO history for all teams and matches after the specified match?",
131
            "ELO history recalculated",
132
            "Recalculate ELOs",
133
            function () use ($match) {
134
                if ($match->isDeleted()) {
135
                    return new RedirectResponse($match->getURL('list'));
136
                }
137
138
                return new RedirectResponse($match->getURL('show'));
139
            },
140
            "Match/recalculate.html.twig",
141
            $noButton = true
142
        );
143
    }
144
145
    /**
146
     * Recalculates match history for all teams and matches
147
     *
148
     * Recalculation is done as follows:
149
     * 1. A match is chosen as a starting point - it's stored old team ELOs are
150
     *    considered correct
151
     * 2. Team ELOs are reset to their values at the starting point
152
     * 3. Each match that occurred since the first specified match has its ELO
153
     *    recalculated based on the current team values, and the new match data
154
     *    and team ELOs are stored in the database
155
     *
156
     * @param Match $match The first match
157
     */
158
    private function recalculate(Match $match)
159
    {
160
        try {
161
            // Commented out to prevent ridiculously large recalculations
162
            //set_time_limit(0);
163
164
            $query = Match::getQueryBuilder()
165
                ->where('status')->notEquals('deleted')
166
                ->where('type')->equals(Match::OFFICIAL)
167
                ->where('time')->isAfter($match->getTimestamp(), $inclusive = true)
168
                ->sortBy('time');
169
170
            /** @var Match[] $matches */
171
            $matches = $query->getModels($fast = true);
172
173
            // Send the total count to client-side javascript
174
            $this->log(count($matches) . "\n");
175
176
            // Start a transaction so tables are locked and we don't stay with
177
            // messed up data if something goes wrong
178
            Database::getInstance()->startTransaction();
179
180
            $teamsReset = [];
181
182
            // Reset match teams, in case the selected match is deleted and does
183
            // not show up in the list of matches to recalculate
184
            $match->getTeamA()->setElo($match->getTeamAEloOld());
0 ignored issues
show
Bug introduced by
It seems like you code against a concrete implementation and not the interface TeamInterface as the method setElo() does only exist in the following implementations of said interface: Team.

Let’s take a look at an example:

interface User
{
    /** @return string */
    public function getPassword();
}

class MyUser implements 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 implementation of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. 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.
  1. Add the method to the interface:

    interface User
    {
        /** @return string */
        public function getPassword();
    
        /** @return string */
        public function getDisplayName();
    }
    
Loading history...
185
            $match->getTeamB()->setElo($match->getTeamBEloOld());
0 ignored issues
show
Bug introduced by
It seems like you code against a concrete implementation and not the interface TeamInterface as the method setElo() does only exist in the following implementations of said interface: Team.

Let’s take a look at an example:

interface User
{
    /** @return string */
    public function getPassword();
}

class MyUser implements 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 implementation of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. 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.
  1. Add the method to the interface:

    interface User
    {
        /** @return string */
        public function getPassword();
    
        /** @return string */
        public function getDisplayName();
    }
    
Loading history...
186
            $teamsReset[ $match->getTeamA()->getId() ] = true;
187
            $teamsReset[ $match->getTeamB()->getId() ] = true;
188
189
            foreach ($matches as $i => $match) {
190
                // Reset teams' ELOs if they haven't been reset already
191 View Code Duplication
                if (!isset($teamsReset[ $match->getTeamA()->getId() ])) {
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across 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...
192
                    $teamsReset[ $match->getTeamA()->getId() ] = true;
193
                    $match->getTeamA()->setElo($match->getTeamAEloOld());
0 ignored issues
show
Bug introduced by
It seems like you code against a concrete implementation and not the interface TeamInterface as the method setElo() does only exist in the following implementations of said interface: Team.

Let’s take a look at an example:

interface User
{
    /** @return string */
    public function getPassword();
}

class MyUser implements 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 implementation of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. 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.
  1. Add the method to the interface:

    interface User
    {
        /** @return string */
        public function getPassword();
    
        /** @return string */
        public function getDisplayName();
    }
    
Loading history...
194
                }
195 View Code Duplication
                if (!isset($teamsReset[ $match->getTeamB()->getId() ])) {
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across 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...
196
                    $teamsReset[ $match->getTeamB()->getId() ] = true;
197
                    $match->getTeamB()->setElo($match->getTeamBEloOld());
0 ignored issues
show
Bug introduced by
It seems like you code against a concrete implementation and not the interface TeamInterface as the method setElo() does only exist in the following implementations of said interface: Team.

Let’s take a look at an example:

interface User
{
    /** @return string */
    public function getPassword();
}

class MyUser implements 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 implementation of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. 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.
  1. Add the method to the interface:

    interface User
    {
        /** @return string */
        public function getPassword();
    
        /** @return string */
        public function getDisplayName();
    }
    
Loading history...
198
                }
199
200
                $match->recalculateElo();
201
202
                // Send an update to the client-side javascript, so that a
203
                // progress bar can be updated
204
                $this->log("m");
205
            }
206
        } catch (Exception $e) {
207
            Database::getInstance()->rollback();
208
            Database::getInstance()->finishTransaction();
209
            throw $e;
210
        }
211
212
        Database::getInstance()->finishTransaction();
213
214
        $this->log("\n\nCalculation successful\n");
215
    }
216
217
    /**
218
     * Echo a string and flush the buffers
219
     *
220
     * Useful for streamed AJAX responses
221
     *
222
     * @param string $string The string to echo
223
     */
224
    private function log($string)
225
    {
226
        echo $string;
227
        ob_flush();
228
        flush();
229
    }
230
231
    /**
232
     * {@inheritdoc}
233
     */
234 1
    protected function getMessages($type, $name = '')
235
    {
236 1
        $messages = parent::getMessages($type, $name);
237
238
        // Don't show the match info on the successful create/edit message
239 1
        foreach ($messages as &$action) {
240 1
            foreach ($action as &$status) {
241 1
                if (isset($status['named'])) {
242 1
                    $status['named'] = $status['unnamed'];
243
                }
244
            }
245
        }
246
247 1
        return $messages;
248
    }
249
250 1
    protected function validate($form)
251
    {
252
        // Make sure that two different teams participated in a match, i.e. a team
253
        // didn't match against itself
254 1
        $firstTeam  = $form->get('first_team')->get('team')->getData();
255 1
        $secondTeam = $form->get('second_team')->get('team')->getData();
256
257 1
        if (!$firstTeam || !$secondTeam) {
258
            return;
259
        }
260
261 1
        if ($firstTeam->isSameAs($secondTeam)) {
262 1
            $message = "You can't report a match where a team played against itself!";
263 1
            $form->addError(new FormError($message));
264
        }
265
266 1
        $matchType = $form->get('type')->getData();
267
268 1
        foreach (array('first_team', 'second_team') as $team) {
269 1
            $input = $form->get($team);
270 1
            $teamInput = $input->get('team');
271 1
            $teamParticipants = $input->get('participants');
272
273 1
            if ($matchType === Match::FUN) {
274 1
                if (!$teamInput->getData() instanceof ColorTeam) {
275
                    $message = "Please enter a team color for fun and special matches.";
276 1
                    $teamInput->addError(new FormError($message));
277
                }
278 1
            } elseif ($matchType === Match::OFFICIAL) {
279 1
                if ($teamInput->getData() instanceof ColorTeam) {
280 1
                    $participants = $teamParticipants->getData();
281
282 1
                    if (empty($participants)) {
283 1
                        $message = 'A player roster is necessary for a color team for a mixed official match.';
284 1
                        $teamInput->addError(new FormError($message));
285
                    }
286
                }
287
            }
288
        }
289 1
    }
290
291
    protected function validateEdit($form, $match)
292
    {
293
        if ($match->isOfficial() && $form->get('type')->getData() !== Match::OFFICIAL) {
294
            $message = "You cannot change this match's type.";
295
            $form->get('type')->addError(new FormError($message));
296
        } elseif (!$match->isOfficial() && $form->get('type')->getData() === Match::OFFICIAL) {
297
            $message = "You can't make this an official match.";
298
            $form->get('type')->addError(new FormError($message));
299
        }
300
    }
301
}
302