Fight - Code Metrics - Inspection of "change getFighterName / short_id to getFighterAttr" - xoco70/laravel-tournaments - Measure and Improve Code Quality continuously with Scrutinizer

Passed

Branch — master (f75c86)

by Julien

created 2017-05-21 06:13 UTC

Fight B

↳ Parent: Project

Complexity

Total Complexity

Size/Duplication

Total Lines	271
Duplicated Lines	6.64 %

Coupling/Cohesion

Components	3
Dependencies	5

Importance

Changes	9
Bugs	1	Features	1

Metric	Value
wmc	51
c	9
b	1
f	1
lcom	3
cbo	5
dl	18
loc	271
rs	8.3206

13 Methods

Rating	Name	Duplication	Size	Complexity
A	__construct()	0	5	1
A	group()	0	4	1
B	getActorsToFights()	12	25	6
A	competitor1()	0	4	1
A	competitor2()	0	4	1
A	team1()	0	4	1
A	team2()	0	4	1
C	savePreliminaryFightGroup()	0	40	8
B	saveGroupFights()	0	30	5
B	getFighterAttr()	0	17	7
B	updateParentFight()	6	13	9
B	getParentFighterToUpdate()	0	16	5
B	getValueToUpdate()	0	13	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:

If you have the same expression in different places: Extract expression to a method
If you have the same method in different sub-classes: Extract method, and pull up field to the parent class
If you have the same code in unrelated classes: Consider extracting the code to a new class, and injecting that class

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 Fight 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 Fight, and based on these observations, apply Extract Interface, too.

<?php

namespace Xoco70\KendoTournaments\Models;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Support\Collection;

class Fight extends Model
{
    /**
     * Fight constructor.
     * @param int $userId1
     * @param int $userId2
     */
    public function __construct($userId1 = null, $userId2 = null)
    {
        $this->c1 = $userId1;
        $this->c2 = $userId2;
    }

    protected $table = 'fight';
    public $timestamps = true;

    protected $fillable = [
        'group_id',
        'c1',
        'c2',
    ];

    /**
     * Get First Fighter.
     *
     * @return \Illuminate\Database\Eloquent\Relations\BelongsTo
     */
    public function group()
    {
        return $this->belongsTo(FightersGroup::class, 'fighters_group_id');
    }

    /**
     * @param Championship $championship
     *
     * @return Collection
     */
    private static function getActorsToFights(Championship $championship, FightersGroup $group = null)
    {
        if ($championship->category->isTeam) {
            $fighters = $group->teams()->get();
function someFunction(A $objectMaybe = null)
{
    if ($objectMaybe instanceof A) {
        $objectMaybe->doSomething();
    }
}
            if (sizeof($fighters) == 0) {

                $fighters->push(new Team());
                $fighters->push(new Team());
            } else if (count($fighters) % 2 != 0) {
                $fighters->push(new Team(['name' => 'BYE']));
            }

        } else {
            $fighters = $group->competitors()->get();
            if (sizeof($fighters) == 0) { // If

                $fighters->push(new Competitor());
                $fighters->push(new Competitor());
            } else if (count($fighters) % 2 != 0) { // If fighter is not pair, add a BYE
                $fighters->push(new Competitor());
            }


        }

        return $fighters;
    }

    /**
     * Get First Fighter.
     *
     * @return \Illuminate\Database\Eloquent\Relations\BelongsTo
     */
    public function competitor1()
    {
        return $this->belongsTo(Competitor::class, 'c1', 'id');
    }

    /**
     * Get Second Fighter.
     *
     * @return \Illuminate\Database\Eloquent\Relations\BelongsTo
     */
    public function competitor2()
    {
        return $this->belongsTo(Competitor::class, 'c2', 'id');
    }

    /**
     * Get First Fighter.
     *
     * @return \Illuminate\Database\Eloquent\Relations\BelongsTo
     */
    public function team1()
    {
        return $this->belongsTo(Team::class, 'c1', 'id');
    }

    /**
     * Get Second Fighter.
     *
     * @return \Illuminate\Database\Eloquent\Relations\BelongsTo
     */
    public function team2()
    {
        return $this->belongsTo(Team::class, 'c2', 'id');
    }

    /**
     * Save a Fight.
     *
     * @param Collection $groups
     * @param int $numGroup
     */
    public static function savePreliminaryFightGroup($groups, $numGroup = 1)
    {
        $competitor1 = $competitor2 = null;
        $order = 1;

        foreach ($groups as $group) {

            if ($group->championship->category->isTeam()) {
                $fighters = $group->teams;
            } else {
                $fighters = $group->competitors;
            }

            $fighter1 = $fighters->get(0);
            $fighter2 = $fighters->get(1);
            $fighter3 = $fighters->get(2);

            switch ($numGroup) {
                case 1:
                    $competitor1 = $fighter1;
                    $competitor2 = $fighter2;
                    break;
                case 2:
                    $competitor1 = $fighter2;
                    $competitor2 = $fighter3;
                    break;
                case 3:
                    $competitor1 = $fighter3;
                    $competitor2 = $fighter1;
                    break;
            }
            $fight = new self();
            $fight->fighters_group_id = $group->id;
            $fight->c1 = $competitor1 != null ? $competitor1->id : null;
            $fight->c2 = $competitor2 != null ? $competitor2->id : null;
            $fight->short_id = $order++;
            $fight->area = $group->area;
            $fight->save();
        }
    }


    /**
     * @param Championship $championship
     */
    public static function saveGroupFights(Championship $championship)
    {
        $order = 1;
        foreach ($championship->fightersGroups()->get() as $group) {
            $fighters = self::getActorsToFights($championship, $group);
            $away = $fighters->splice(count($fighters) / 2); // 2
            $home = $fighters; // 1

            for ($i = 0; $i < count($home) + count($away) - 1; $i++) { // 0 -> 2

                for ($j = 0; $j < count($home); $j++) {  // 1 no mas
for ($i=0; $i<count($array); $i++) { // calls count() on each iteration
}

// Better
for ($i=0, $c=count($array); $i<$c; $i++) { // calls count() just once
}

                    $round[$i][$j]['Home'] = $home[$j];
foreach ($collection as $item) {
    $myArray['foo'] = $item->getFoo();

    if ($item->hasBar()) {
        $myArray['bar'] = $item->getBar();
    }

    // do something with $myArray
}
                    $round[$i][$j]['Away'] = $away[$j];
function myFunction($a) {
    switch ($a) {
        case 'foo':
            $x = 1;
            break;

        case 'bar':
            $x = 2;
            break;
    }

    // $x is potentially undefined here.
    echo $x;
}
                    $fight = new self();
                    $fight->fighters_group_id = $group->id;
                    $fight->c1 = $round[$i][$j]['Home']->id;
                    $fight->c2 = $round[$i][$j]['Away']->id;
                    $fight->short_id = $order++;
                    $fight->area = $group->area;
                    $fight->save();

                }
                if (count($home) + count($away) - 1 > 2) {
                    $away->prepend($home->splice(1, 1)->shift());
                    $home->push($away->pop());
                    $order++;
                }
            }
        }
    }

    public function getFighterAttr($numFighter, $attr)
    {
        $isTeam = $this->group->championship->category->isTeam;
        if ($isTeam) {
            $teamToUpdate = 'team' . $numFighter;
            return $this->$teamToUpdate == null ? '' : $this->$teamToUpdate->$attr;
        }
        $competitorToUpdate = 'competitor' . $numFighter;
        if ($attr == 'name') {
            return $this->$competitorToUpdate == null
                ? 'BYE'
                : $this->$competitorToUpdate->user->firstname . " " . $this->$competitorToUpdate->user->lastname;
        } elseif ($attr == 'short_id') {
            return $this->$competitorToUpdate == null ? '' : $this->$competitorToUpdate->short_id;
        }
        return null;
    }

//    public function getFighterShortId($numFighter,$attr)

//    {
//        $isTeam = $this->group->championship->category->isTeam;
//        if ($isTeam) {
//            $teamToUpdate = 'team' . $numFighter;
//            return $this->$teamToUpdate == null ? '' : $this->$teamToUpdate->$attr;
//        }
//        $competitorToUpdate = 'competitor' . $numFighter;
//        return $this->$competitorToUpdate == null ? '' : $this->$competitorToUpdate->short_id;
//    }

    /**
     * Update parent Fight
     */
    public function updateParentFight($fighterToUpdate, $fight)
    {

        if ($fight != null && ($fight->c1 != null || $fight->c2 == null)) {

            $this->$fighterToUpdate = $fight->c1;
        }
        if ($fight != null && $fight->c1 == null || $fight->c2 != null) {

            $this->$fighterToUpdate = $fight->c2;
        }
        if ($fight->c1 == null || $fight->c2 == null) {
            $this->$fighterToUpdate = null;
        }
    }

    /**
     * Returns the parent field that need to be updated
     * @return null|string
     */
    public function getParentFighterToUpdate()
    {
        $childrenGroup = $this->group->parent->children;
        foreach ($childrenGroup as $key => $children) {
            $childFight = $children->fights->get(0);
            if ($childFight->id == $this->id) {
                if ($key % 2 == 0) {
                    return "c1";
                }
                if ($key % 2 == 1) {
                    return "c2";
                }
            }
        }
        return null;
    }

    /**
     * In the original fight ( child ) return the field that contains data to copy to parent
     * @return null|string
     */
    public function getValueToUpdate()
    {
        if ($this->c1 != null && $this->c2 != null) {
            return null;
        }
        if ($this->c1 != null) {
            return "c1";
        }
        if ($this->c2 != null) {
            return "c2";
        }
        return null;
    }

}

1		<?php
2
3		namespace Xoco70\KendoTournaments\Models;
4
5		use Illuminate\Database\Eloquent\Model;
6		use Illuminate\Support\Collection;
7
8		class Fight extends Model
9		{
10		/**
11		* Fight constructor.
12		* @param int $userId1
13		* @param int $userId2
14		*/
15		public function __construct($userId1 = null, $userId2 = null)
16		{
17		$this->c1 = $userId1;
18		$this->c2 = $userId2;
19		}
20
21		protected $table = 'fight';
22		public $timestamps = true;
23
24		protected $fillable = [
25		'group_id',
26		'c1',
27		'c2',
28		];
29
30		/**
31		* Get First Fighter.
32		*
33		* @return \Illuminate\Database\Eloquent\Relations\BelongsTo
34		*/
35		public function group()
36		{
37		return $this->belongsTo(FightersGroup::class, 'fighters_group_id');
38		}
39
40		/**
41		* @param Championship $championship
42		*
43		* @return Collection
44		*/
45		private static function getActorsToFights(Championship $championship, FightersGroup $group = null)
46		{
47		if ($championship->category->isTeam) {
48		$fighters = $group->teams()->get();
		0 ignored issues – show Bug introduced 2017-05-05 23:14 UTC by Report Bug Copy Issue Report It seems like `$group` is not always an object, but can also be of type `null`. Maybe add an additional type check? If a variable is not always an object, we recommend to add an additional type check to ensure your method call is safe: function someFunction(A $objectMaybe = null) { if ($objectMaybe instanceof A) { $objectMaybe->doSomething(); } } Loading history...
49	View Code Duplication	if (sizeof($fighters) == 0) {
		0 ignored issues – show Duplication introduced 2017-03-21 21:30 UTC by Report Bug Copy Issue Report 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...
50		$fighters->push(new Team());
51		$fighters->push(new Team());
52		} else if (count($fighters) % 2 != 0) {
53		$fighters->push(new Team(['name' => 'BYE']));
54		}
55
56		} else {
57		$fighters = $group->competitors()->get();
58	View Code Duplication	if (sizeof($fighters) == 0) { // If
		0 ignored issues – show Duplication introduced 2017-03-21 21:30 UTC by Report Bug Copy Issue Report 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...
59		$fighters->push(new Competitor());
60		$fighters->push(new Competitor());
61		} else if (count($fighters) % 2 != 0) { // If fighter is not pair, add a BYE
62		$fighters->push(new Competitor());
63		}
64
65
66		}
67
68		return $fighters;
69		}
70
71		/**
72		* Get First Fighter.
73		*
74		* @return \Illuminate\Database\Eloquent\Relations\BelongsTo
75		*/
76		public function competitor1()
77		{
78		return $this->belongsTo(Competitor::class, 'c1', 'id');
79		}
80
81		/**
82		* Get Second Fighter.
83		*
84		* @return \Illuminate\Database\Eloquent\Relations\BelongsTo
85		*/
86		public function competitor2()
87		{
88		return $this->belongsTo(Competitor::class, 'c2', 'id');
89		}
90
91		/**
92		* Get First Fighter.
93		*
94		* @return \Illuminate\Database\Eloquent\Relations\BelongsTo
95		*/
96		public function team1()
97		{
98		return $this->belongsTo(Team::class, 'c1', 'id');
99		}
100
101		/**
102		* Get Second Fighter.
103		*
104		* @return \Illuminate\Database\Eloquent\Relations\BelongsTo
105		*/
106		public function team2()
107		{
108		return $this->belongsTo(Team::class, 'c2', 'id');
109		}
110
111		/**
112		* Save a Fight.
113		*
114		* @param Collection $groups
115		* @param int $numGroup
116		*/
117		public static function savePreliminaryFightGroup($groups, $numGroup = 1)
118		{
119		$competitor1 = $competitor2 = null;
120		$order = 1;
121
122		foreach ($groups as $group) {
123
124		if ($group->championship->category->isTeam()) {
125		$fighters = $group->teams;
126		} else {
127		$fighters = $group->competitors;
128		}
129
130		$fighter1 = $fighters->get(0);
131		$fighter2 = $fighters->get(1);
132		$fighter3 = $fighters->get(2);
133
134		switch ($numGroup) {
135		case 1:
136		$competitor1 = $fighter1;
137		$competitor2 = $fighter2;
138		break;
139		case 2:
140		$competitor1 = $fighter2;
141		$competitor2 = $fighter3;
142		break;
143		case 3:
144		$competitor1 = $fighter3;
145		$competitor2 = $fighter1;
146		break;
147		}
148		$fight = new self();
149		$fight->fighters_group_id = $group->id;
150		$fight->c1 = $competitor1 != null ? $competitor1->id : null;
151		$fight->c2 = $competitor2 != null ? $competitor2->id : null;
152		$fight->short_id = $order++;
153		$fight->area = $group->area;
154		$fight->save();
155		}
156		}
157
158
159		/**
160		* @param Championship $championship
161		*/
162		public static function saveGroupFights(Championship $championship)
163		{
164		$order = 1;
165		foreach ($championship->fightersGroups()->get() as $group) {
166		$fighters = self::getActorsToFights($championship, $group);
167		$away = $fighters->splice(count($fighters) / 2); // 2
168		$home = $fighters; // 1
169
170		for ($i = 0; $i < count($home) + count($away) - 1; $i++) { // 0 -> 2
		0 ignored issues – show Unused Code Comprehensibility introduced 2017-02-18 00:44 UTC by Report Bug Copy Issue Report `50%` of this comment could be valid code. Did you maybe forget this after debugging? Sometimes obsolete code just ends up commented out instead of removed. In this case it is better to remove the code once you have checked you do not need it. The code might also have been commented out for debugging purposes. In this case it is vital that someone uncomments it again or your project may behave in very unexpected ways in production. This check looks for comments that seem to be mostly valid code and reports them. Loading history...
171		for ($j = 0; $j < count($home); $j++) { // 1 no mas
		0 ignored issues – show Performance Best Practice introduced 2017-02-18 00:44 UTC by Report Bug Copy Issue Report It seems like you are calling the size function `count()` as part of the test condition. You might want to compute the size beforehand, and not on each iteration. If the size of the collection does not change during the iteration, it is generally a good practice to compute it beforehand, and not on each iteration: for ($i=0; $i<count($array); $i++) { // calls count() on each iteration } // Better for ($i=0, $c=count($array); $i<$c; $i++) { // calls count() just once } Loading history...
172
173		$round[$i][$j]['Home'] = $home[$j];
		0 ignored issues – show Coding Style Comprehensibility introduced 2017-02-12 04:09 UTC by Report Bug Copy Issue Report `$round` was never initialized. Although not strictly required by PHP, it is generally a good practice to add `$round = array();` before regardless. Adding an explicit array definition is generally preferable to implicit array definition as it guarantees a stable state of the code. Let’s take a look at an example: foreach ($collection as $item) { $myArray['foo'] = $item->getFoo(); if ($item->hasBar()) { $myArray['bar'] = $item->getBar(); } // do something with $myArray } As you can see in this example, the array `$myArray` is initialized the first time when the foreach loop is entered. You can also see that the value of the `bar` key is only written conditionally; thus, its value might result from a previous iteration. This might or might not be intended. To make your intention clear, your code more readible and to avoid accidental bugs, we recommend to add an explicit initialization `$myArray = array()` either outside or inside the foreach loop. Loading history...
174		$round[$i][$j]['Away'] = $away[$j];
		0 ignored issues – show Bug introduced 2017-02-12 04:09 UTC by Report Bug Copy Issue Report The variable `$round` does not seem to be defined for all execution paths leading up to this point. If you define a variable conditionally, it can happen that it is not defined for all execution paths. Let’s take a look at an example: function myFunction($a) { switch ($a) { case 'foo': $x = 1; break; case 'bar': $x = 2; break; } // $x is potentially undefined here. echo $x; } In the above example, the variable `$x` is defined if you pass “foo” or “bar” as argument for `$a`. However, since the `switch` statement has no default case statement, if you pass any other value, the variable `$x` would be undefined. Available Fixes Check for existence of the variable explicitly: function myFunction($a) { switch ($a) { case 'foo': $x = 1; break; case 'bar': $x = 2; break; } if (isset($x)) { // Make sure it's always set. echo $x; } } Define a default value for the variable: function myFunction($a) { $x = ''; // Set a default which gets overridden for certain paths. switch ($a) { case 'foo': $x = 1; break; case 'bar': $x = 2; break; } echo $x; } Add a value for the missing path: function myFunction($a) { switch ($a) { case 'foo': $x = 1; break; case 'bar': $x = 2; break; // We add support for the missing case. default: $x = ''; break; } echo $x; } Loading history...
175		$fight = new self();
176		$fight->fighters_group_id = $group->id;
177		$fight->c1 = $round[$i][$j]['Home']->id;
178		$fight->c2 = $round[$i][$j]['Away']->id;
179		$fight->short_id = $order++;
180		$fight->area = $group->area;
181		$fight->save();
182
183		}
184		if (count($home) + count($away) - 1 > 2) {
185		$away->prepend($home->splice(1, 1)->shift());
186		$home->push($away->pop());
187		$order++;
188		}
189		}
190		}
191		}
192
193		public function getFighterAttr($numFighter, $attr)
194		{
195		$isTeam = $this->group->championship->category->isTeam;
196		if ($isTeam) {
197		$teamToUpdate = 'team' . $numFighter;
198		return $this->$teamToUpdate == null ? '' : $this->$teamToUpdate->$attr;
199		}
200		$competitorToUpdate = 'competitor' . $numFighter;
201		if ($attr == 'name') {
202		return $this->$competitorToUpdate == null
203		? 'BYE'
204		: $this->$competitorToUpdate->user->firstname . " " . $this->$competitorToUpdate->user->lastname;
205		} elseif ($attr == 'short_id') {
206		return $this->$competitorToUpdate == null ? '' : $this->$competitorToUpdate->short_id;
207		}
208		return null;
209		}
210
211		// public function getFighterShortId($numFighter,$attr)
		0 ignored issues – show Unused Code Comprehensibility introduced 2017-05-21 06:45 UTC by Report Bug Copy Issue Report `49%` of this comment could be valid code. Did you maybe forget this after debugging? Sometimes obsolete code just ends up commented out instead of removed. In this case it is better to remove the code once you have checked you do not need it. The code might also have been commented out for debugging purposes. In this case it is vital that someone uncomments it again or your project may behave in very unexpected ways in production. This check looks for comments that seem to be mostly valid code and reports them. Loading history...
212		// {
213		// $isTeam = $this->group->championship->category->isTeam;
214		// if ($isTeam) {
215		// $teamToUpdate = 'team' . $numFighter;
216		// return $this->$teamToUpdate == null ? '' : $this->$teamToUpdate->$attr;
217		// }
218		// $competitorToUpdate = 'competitor' . $numFighter;
219		// return $this->$competitorToUpdate == null ? '' : $this->$competitorToUpdate->short_id;
220		// }
221
222		/**
223		* Update parent Fight
224		*/
225		public function updateParentFight($fighterToUpdate, $fight)
226		{
227
228	View Code Duplication	if ($fight != null && ($fight->c1 != null \|\| $fight->c2 == null)) {
		0 ignored issues – show Duplication introduced 2017-05-10 03:44 UTC by Report Bug Copy Issue Report 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...
229		$this->$fighterToUpdate = $fight->c1;
230		}
231	View Code Duplication	if ($fight != null && $fight->c1 == null \|\| $fight->c2 != null) {
		0 ignored issues – show Duplication introduced 2017-05-10 03:44 UTC by Report Bug Copy Issue Report 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...
232		$this->$fighterToUpdate = $fight->c2;
233		}
234		if ($fight->c1 == null \|\| $fight->c2 == null) {
235		$this->$fighterToUpdate = null;
236		}
237		}
238
239		/**
240		* Returns the parent field that need to be updated
241		* @return null\|string
242		*/
243		public function getParentFighterToUpdate()
244		{
245		$childrenGroup = $this->group->parent->children;
246		foreach ($childrenGroup as $key => $children) {
247		$childFight = $children->fights->get(0);
248		if ($childFight->id == $this->id) {
249		if ($key % 2 == 0) {
250		return "c1";
251		}
252		if ($key % 2 == 1) {
253		return "c2";
254		}
255		}
256		}
257		return null;
258		}
259
260		/**
261		* In the original fight ( child ) return the field that contains data to copy to parent
262		* @return null\|string
263		*/
264		public function getValueToUpdate()
265		{
266		if ($this->c1 != null && $this->c2 != null) {
267		return null;
268		}
269		if ($this->c1 != null) {
270		return "c1";
271		}
272		if ($this->c2 != null) {
273		return "c2";
274		}
275		return null;
276		}
277
278		}

xoco70 / laravel-tournaments