Row::saveExternalRelations() - Code Metrics - Inspection of "isset fix" - oscarotero/simple-crud - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Push — master ( 0fe8b6...bcfab5 )

by Oscar

created 2015-12-09 19:13 UTC

Row::saveExternalRelations() C

↳ Parent: Row

Complexity

Conditions	8
Paths	11

Size

Total Lines	51
Code Lines	30

Duplication

Lines	0
Ratio	0 %

Importance

Changes	3
Bugs	2	Features	0

Metric	Value
c	3
b	2
f	0
dl	0
loc	51
rs	6.5979
cc	8
eloc	30
nc	11
nop	0

How to fix Long Method

<?php

namespace SimpleCrud;

use JsonSerializable;

/**
 * Stores the data of an entity row.
 */
class Row extends BaseRow implements JsonSerializable
{
    private $values = [];

    /**
     * {@inheritdoc}
     */
    public function __construct(Entity $entity)
    {
        parent::__construct($entity);

        $this->values = array_fill_keys(array_keys($entity->fields), null);
    }

    /**
     * Magic method to return properties or load them automatically.
     *
     * @param string $name
     */
    public function __get($name)
    {
        //Return properties
        if (array_key_exists($name, $this->values)) {
            return $this->values[$name];
        }

        //Custom property
        if (isset($this->properties[$name])) {
            return $this->values[$name] = call_user_func($this->properties[$name], $this);
        }

        //Load related data
        switch ($this->entity->getRelation($name)) {
            case Entity::RELATION_HAS_ONE:
                return $this->values[$name] = $this->select($name)->one() ?: new NullValue();
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.
    }
}

            case Entity::RELATION_HAS_MANY:
            case Entity::RELATION_HAS_BRIDGE:
                return $this->values[$name] = $this->select($name)->all();
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.
    }
}
        }
    }

    /**
     * Magic method to store properties.
     *
     * @param string $name
     * @param mixed  $value
     */
    public function __set($name, $value)
    {
        $this->values[$name] = $value;
    }

    /**
     * Magic method to check if a property is defined or is empty.
     *
     * @param string $name Property name
     *
     * @return bool
     */
    public function __isset($name)
    {
        return !empty($this->values[$name]) && !($this->values[$name] instanceof NullValue);
    }

    /**
     * Magic method to print the row values (and subvalues).
     *
     * @return string
     */
    public function __toString()
    {
        return "\n".$this->getEntity()->name.":\n".print_r($this->toArray(), true)."\n";
    }

    /**
     * @see RowInterface
     *
     * {@inheritdoc}
     */
    public function toArray($keysAsId = false, array $parentEntities = array())
    {
        if (!empty($parentEntities) && in_array($this->entity->name, $parentEntities)) {
            return;
        }

        $parentEntities[] = $this->entity->name;
        $data = $this->values;

        foreach ($data as &$value) {
            if ($value instanceof RowInterface) {
                $value = $value->toArray($keysAsId, $parentEntities);
            }
        }

        return $data;
    }

    /**
     * Relate 'has-one' elements with this row.
     *
     * @param RowInterface $row The row to relate
     *
     * @return $this
     */
    public function relateWith(RowInterface $row)
    {
        if (!$this->entity->hasOne($row->getEntity())) {
            throw new SimpleCrudException('Not valid relation');
        }

        if (empty($row->id)) {
interface SomeInterface { }
class SomeClass implements SomeInterface {
    public $a;
}

function someFunction(SomeInterface $object) {
    if ($object instanceof SomeClass) {
        $a = $object->a;
    }
}
            throw new SimpleCrudException('Rows without id value cannot be related');
        }

        $this->{$row->getEntity()->foreignKey} = $row->id;
interface SomeInterface { }
class SomeClass implements SomeInterface {
    public $a;
}

function someFunction(SomeInterface $object) {
    if ($object instanceof SomeClass) {
        $a = $object->a;
    }
}

        return $this;
    }

    /**
     * Set new values to the row.
     *
     * @param array $data The new values
     *
     * @return $this
     */
    public function set(array $data)
    {
        $this->values = $data + $this->values;

        return $this;
    }

    /**
     * Return one or all values of the row.
     *
     * @param null|string $name The value name to recover. If it's not defined, returns all values.
     *
     * @return mixed
     */
    public function get($name = null)
    {
        if ($name === null) {
            return $this->values;
        }

        return isset($this->values[$name]) ? $this->values[$name] : null;
    }

    /**
     * Return whether a value is defined or not.
     *
     * @param string $name
     *
     * @return bool
     */
    public function has($name)
    {
        return array_key_exists($name, $this->values);
    }

    /**
     * Saves this row in the database.
     *
     * @param bool $duplications      Set true to detect duplicates index
     * @param bool $externalRelations Set true to save the relations with other entities
     *
     * @return $this
     */
    public function save($duplications = false, $externalRelations = false)
    {
        $data = array_intersect_key($this->values, $this->entity->fields);

        if (empty($this->id)) {
            $this->id = $this->entity->insert()
                ->data($data)
                ->duplications($duplications)
                ->get();
        } else {
            $this->entity->update()
                ->data($data)
                ->byId($this->id)
                ->limit(1)
                ->run();
        }

        if ($externalRelations) {
            $this->saveExternalRelations();
        }

        return $this;
    }

    /**
     * Saves the extenal relations (RELATION_HAS_MANY|RELATION_HAS_BRIDGE) of this row with other row directly in the database.
     *
     * @return $this
     */
    protected function saveExternalRelations()
    {
        $extData = array_diff_key($this->values, $this->entity->fields);
        $db = $this->entity->getDb();

        foreach ($extData as $name => $ids) {
            if (!$db->has($name)) {
                continue;
            }

            $entity = $db->get($name);

            if ($this->entity->hasOne($entity)) {
/** @return stdClass|null */
function mayReturnNull() { }

function doesNotAcceptNull(stdClass $x) { }

// With potential error.
function withoutCheck() {
    $x = mayReturnNull();
    doesNotAcceptNull($x); // Potential error here.
}

// Safe - Alternative 1
function withCheck1() {
    $x = mayReturnNull();
    if ( ! $x instanceof stdClass) {
        throw new \LogicException('$x must be defined.');
    }
    doesNotAcceptNull($x);
}

// Safe - Alternative 2
function withCheck2() {
    $x = mayReturnNull();
    if ($x instanceof stdClass) {
        doesNotAcceptNull($x);
    }
}
                continue;
            }

            $ids = ($ids instanceof RowInterface) ? (array) $ids->id : (array) $ids;

            if ($this->entity->hasMany($entity)) {
/** @return stdClass|null */
function mayReturnNull() { }

function doesNotAcceptNull(stdClass $x) { }

// With potential error.
function withoutCheck() {
    $x = mayReturnNull();
    doesNotAcceptNull($x); // Potential error here.
}

// Safe - Alternative 1
function withCheck1() {
    $x = mayReturnNull();
    if ( ! $x instanceof stdClass) {
        throw new \LogicException('$x must be defined.');
    }
    doesNotAcceptNull($x);
}

// Safe - Alternative 2
function withCheck2() {
    $x = mayReturnNull();
    if ($x instanceof stdClass) {
        doesNotAcceptNull($x);
    }
}
                $entity->update()
                    ->data([
                        $this->entity->foreignKey => $this->id,
                    ])
                    ->by('id', $ids)
                    ->run();

                continue;
            }

            if ($this->entity->hasBridge($entity)) {
/** @return stdClass|null */
function mayReturnNull() { }

function doesNotAcceptNull(stdClass $x) { }

// With potential error.
function withoutCheck() {
    $x = mayReturnNull();
    doesNotAcceptNull($x); // Potential error here.
}

// Safe - Alternative 1
function withCheck1() {
    $x = mayReturnNull();
    if ( ! $x instanceof stdClass) {
        throw new \LogicException('$x must be defined.');
    }
    doesNotAcceptNull($x);
}

// Safe - Alternative 2
function withCheck2() {
    $x = mayReturnNull();
    if ($x instanceof stdClass) {
        doesNotAcceptNull($x);
    }
}
                $bridge = $this->entity->getBridge($entity);
/** @return stdClass|null */
function mayReturnNull() { }

function doesNotAcceptNull(stdClass $x) { }

// With potential error.
function withoutCheck() {
    $x = mayReturnNull();
    doesNotAcceptNull($x); // Potential error here.
}

// Safe - Alternative 1
function withCheck1() {
    $x = mayReturnNull();
    if ( ! $x instanceof stdClass) {
        throw new \LogicException('$x must be defined.');
    }
    doesNotAcceptNull($x);
}

// Safe - Alternative 2
function withCheck2() {
    $x = mayReturnNull();
    if ($x instanceof stdClass) {
        doesNotAcceptNull($x);
    }
}

                $bridge->delete()
                    ->by($this->entity->foreignKey, $this->id)
                    ->run();

                foreach ($ids as $id) {
                    $bridge->insert()
                        ->data([
                            $this->entity->foreignKey => $this->id,
                            $entity->foreignKey => $id,
                        ])
                        ->run();
                }

                continue;
            }
        }

        return $this;
    }
}


Push — master ( 0fe8b6...bcfab5 )

Row::saveExternalRelations() C

Complexity

Size

Duplication

Importance

How to fix Long Method

Long Method

Available Fixes

Available Fixes

Available Fixes

Available Fixes

1			<?php
2
3			namespace SimpleCrud;
4
5			use JsonSerializable;
6
7			/**
8			* Stores the data of an entity row.
9			*/
10			class Row extends BaseRow implements JsonSerializable
11			{
12			private $values = [];
13
14			/**
15			* {@inheritdoc}
16			*/
17			public function __construct(Entity $entity)
18			{
19			parent::__construct($entity);
20
21			$this->values = array_fill_keys(array_keys($entity->fields), null);
22			}
23
24			/**
25			* Magic method to return properties or load them automatically.
26			*
27			* @param string $name
28			*/
29			public function __get($name)
30			{
31			//Return properties
32			if (array_key_exists($name, $this->values)) {
33			return $this->values[$name];
34			}
35
36			//Custom property
37			if (isset($this->properties[$name])) {
38			return $this->values[$name] = call_user_func($this->properties[$name], $this);
39			}
40
41			//Load related data
42			switch ($this->entity->getRelation($name)) {
43			case Entity::RELATION_HAS_ONE:
44			return $this->values[$name] = $this->select($name)->one() ?: new NullValue();
			0 ignored issues – show Bug introduced 2015-12-02 18:20 UTC by Report Bug Copy Issue Report It seems like you code against a concrete implementation and not the interface `SimpleCrud\QueryInterface` as the method `one()` does only exist in the following implementations of said interface: `SimpleCrud\Queries\Mysql\Select`, `SimpleCrud\Queries\Sqlite\Select`. 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 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 interface: interface User { /** @return string / public function getPassword(); /* @return string */ public function getDisplayName(); } Loading history...
45
46			case Entity::RELATION_HAS_MANY:
47			case Entity::RELATION_HAS_BRIDGE:
48			return $this->values[$name] = $this->select($name)->all();
			0 ignored issues – show Bug introduced 2015-12-02 18:20 UTC by Report Bug Copy Issue Report It seems like you code against a concrete implementation and not the interface `SimpleCrud\QueryInterface` as the method `all()` does only exist in the following implementations of said interface: `SimpleCrud\Queries\Mysql\Select`, `SimpleCrud\Queries\Sqlite\Select`. 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 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 interface: interface User { /** @return string / public function getPassword(); /* @return string */ public function getDisplayName(); } Loading history...
49			}
50			}
51
52			/**
53			* Magic method to store properties.
54			*
55			* @param string $name
56			* @param mixed $value
57			*/
58			public function __set($name, $value)
59			{
60			$this->values[$name] = $value;
61			}
62
63			/**
64			* Magic method to check if a property is defined or is empty.
65			*
66			* @param string $name Property name
67			*
68			* @return bool
69			*/
70			public function __isset($name)
71			{
72			return !empty($this->values[$name]) && !($this->values[$name] instanceof NullValue);
73			}
74
75			/**
76			* Magic method to print the row values (and subvalues).
77			*
78			* @return string
79			*/
80			public function __toString()
81			{
82			return "\n".$this->getEntity()->name.":\n".print_r($this->toArray(), true)."\n";
83			}
84
85			/**
86			* @see RowInterface
87			*
88			* {@inheritdoc}
89			*/
90			public function toArray($keysAsId = false, array $parentEntities = array())
91			{
92			if (!empty($parentEntities) && in_array($this->entity->name, $parentEntities)) {
93			return;
94			}
95
96			$parentEntities[] = $this->entity->name;
97			$data = $this->values;
98
99			foreach ($data as &$value) {
100			if ($value instanceof RowInterface) {
101			$value = $value->toArray($keysAsId, $parentEntities);
102			}
103			}
104
105			return $data;
106			}
107
108			/**
109			* Relate 'has-one' elements with this row.
110			*
111			* @param RowInterface $row The row to relate
112			*
113			* @return $this
114			*/
115			public function relateWith(RowInterface $row)
116			{
117			if (!$this->entity->hasOne($row->getEntity())) {
118			throw new SimpleCrudException('Not valid relation');
119			}
120
121			if (empty($row->id)) {
			0 ignored issues – show Bug introduced 2015-12-02 18:20 UTC by Report Bug Copy Issue Report Accessing `id` on the interface `SimpleCrud\RowInterface` suggest that you code against a concrete implementation. How about adding an `instanceof` check? If you access a property on an interface, you most likely code against a concrete implementation of the interface. Available Fixes Adding an additional type check: interface SomeInterface { } class SomeClass implements SomeInterface { public $a; } function someFunction(SomeInterface $object) { if ($object instanceof SomeClass) { $a = $object->a; } } Changing the type hint: interface SomeInterface { } class SomeClass implements SomeInterface { public $a; } function someFunction(SomeClass $object) { $a = $object->a; } Loading history...
122			throw new SimpleCrudException('Rows without id value cannot be related');
123			}
124
125			$this->{$row->getEntity()->foreignKey} = $row->id;
			0 ignored issues – show Bug introduced 2015-12-02 18:20 UTC by Report Bug Copy Issue Report Accessing `id` on the interface `SimpleCrud\RowInterface` suggest that you code against a concrete implementation. How about adding an `instanceof` check? If you access a property on an interface, you most likely code against a concrete implementation of the interface. Available Fixes Adding an additional type check: interface SomeInterface { } class SomeClass implements SomeInterface { public $a; } function someFunction(SomeInterface $object) { if ($object instanceof SomeClass) { $a = $object->a; } } Changing the type hint: interface SomeInterface { } class SomeClass implements SomeInterface { public $a; } function someFunction(SomeClass $object) { $a = $object->a; } Loading history...
126
127			return $this;
128			}
129
130			/**
131			* Set new values to the row.
132			*
133			* @param array $data The new values
134			*
135			* @return $this
136			*/
137			public function set(array $data)
138			{
139			$this->values = $data + $this->values;
140
141			return $this;
142			}
143
144			/**
145			* Return one or all values of the row.
146			*
147			* @param null\|string $name The value name to recover. If it's not defined, returns all values.
148			*
149			* @return mixed
150			*/
151			public function get($name = null)
152			{
153			if ($name === null) {
154			return $this->values;
155			}
156
157			return isset($this->values[$name]) ? $this->values[$name] : null;
158			}
159
160			/**
161			* Return whether a value is defined or not.
162			*
163			* @param string $name
164			*
165			* @return bool
166			*/
167			public function has($name)
168			{
169			return array_key_exists($name, $this->values);
170			}
171
172			/**
173			* Saves this row in the database.
174			*
175			* @param bool $duplications Set true to detect duplicates index
176			* @param bool $externalRelations Set true to save the relations with other entities
177			*
178			* @return $this
179			*/
180			public function save($duplications = false, $externalRelations = false)
181			{
182			$data = array_intersect_key($this->values, $this->entity->fields);
183
184			if (empty($this->id)) {
185			$this->id = $this->entity->insert()
186			->data($data)
187			->duplications($duplications)
188			->get();
189			} else {
190			$this->entity->update()
191			->data($data)
192			->byId($this->id)
193			->limit(1)
194			->run();
195			}
196
197			if ($externalRelations) {
198			$this->saveExternalRelations();
199			}
200
201			return $this;
202			}
203
204			/**
205			* Saves the extenal relations (RELATION_HAS_MANY\|RELATION_HAS_BRIDGE) of this row with other row directly in the database.
206			*
207			* @return $this
208			*/
209			protected function saveExternalRelations()
210			{
211			$extData = array_diff_key($this->values, $this->entity->fields);
212			$db = $this->entity->getDb();
213
214			foreach ($extData as $name => $ids) {
215			if (!$db->has($name)) {
216			continue;
217			}
218
219			$entity = $db->get($name);
220
221			if ($this->entity->hasOne($entity)) {
			0 ignored issues – show Bug introduced 2015-12-02 18:20 UTC by Report Bug Copy Issue Report It seems like `$entity` defined by `$db->get($name)` on line `219` can be `null`; however, `SimpleCrud\Entity::hasOne()` does not accept `null`, maybe add an additional type check? Unless you are absolutely sure that the expression can never be null because of other conditions, we strongly recommend to add an additional type check to your code: /** @return stdClass\|null */ function mayReturnNull() { } function doesNotAcceptNull(stdClass $x) { } // With potential error. function withoutCheck() { $x = mayReturnNull(); doesNotAcceptNull($x); // Potential error here. } // Safe - Alternative 1 function withCheck1() { $x = mayReturnNull(); if ( ! $x instanceof stdClass) { throw new \LogicException('$x must be defined.'); } doesNotAcceptNull($x); } // Safe - Alternative 2 function withCheck2() { $x = mayReturnNull(); if ($x instanceof stdClass) { doesNotAcceptNull($x); } } Loading history...
222			continue;
223			}
224
225			$ids = ($ids instanceof RowInterface) ? (array) $ids->id : (array) $ids;
226
227			if ($this->entity->hasMany($entity)) {
			0 ignored issues – show Bug introduced 2015-12-02 18:20 UTC by Report Bug Copy Issue Report It seems like `$entity` defined by `$db->get($name)` on line `219` can be `null`; however, `SimpleCrud\Entity::hasMany()` does not accept `null`, maybe add an additional type check? Unless you are absolutely sure that the expression can never be null because of other conditions, we strongly recommend to add an additional type check to your code: /** @return stdClass\|null */ function mayReturnNull() { } function doesNotAcceptNull(stdClass $x) { } // With potential error. function withoutCheck() { $x = mayReturnNull(); doesNotAcceptNull($x); // Potential error here. } // Safe - Alternative 1 function withCheck1() { $x = mayReturnNull(); if ( ! $x instanceof stdClass) { throw new \LogicException('$x must be defined.'); } doesNotAcceptNull($x); } // Safe - Alternative 2 function withCheck2() { $x = mayReturnNull(); if ($x instanceof stdClass) { doesNotAcceptNull($x); } } Loading history...
228			$entity->update()
229			->data([
230			$this->entity->foreignKey => $this->id,
231			])
232			->by('id', $ids)
233			->run();
234
235			continue;
236			}
237
238			if ($this->entity->hasBridge($entity)) {
			0 ignored issues – show Bug introduced 2015-12-02 18:20 UTC by Report Bug Copy Issue Report It seems like `$entity` defined by `$db->get($name)` on line `219` can be `null`; however, `SimpleCrud\Entity::hasBridge()` does not accept `null`, maybe add an additional type check? Unless you are absolutely sure that the expression can never be null because of other conditions, we strongly recommend to add an additional type check to your code: /** @return stdClass\|null */ function mayReturnNull() { } function doesNotAcceptNull(stdClass $x) { } // With potential error. function withoutCheck() { $x = mayReturnNull(); doesNotAcceptNull($x); // Potential error here. } // Safe - Alternative 1 function withCheck1() { $x = mayReturnNull(); if ( ! $x instanceof stdClass) { throw new \LogicException('$x must be defined.'); } doesNotAcceptNull($x); } // Safe - Alternative 2 function withCheck2() { $x = mayReturnNull(); if ($x instanceof stdClass) { doesNotAcceptNull($x); } } Loading history...
239			$bridge = $this->entity->getBridge($entity);
			0 ignored issues – show Bug introduced 2015-12-02 18:20 UTC by Report Bug Copy Issue Report It seems like `$entity` defined by `$db->get($name)` on line `219` can be `null`; however, `SimpleCrud\Entity::getBridge()` does not accept `null`, maybe add an additional type check? Unless you are absolutely sure that the expression can never be null because of other conditions, we strongly recommend to add an additional type check to your code: /** @return stdClass\|null */ function mayReturnNull() { } function doesNotAcceptNull(stdClass $x) { } // With potential error. function withoutCheck() { $x = mayReturnNull(); doesNotAcceptNull($x); // Potential error here. } // Safe - Alternative 1 function withCheck1() { $x = mayReturnNull(); if ( ! $x instanceof stdClass) { throw new \LogicException('$x must be defined.'); } doesNotAcceptNull($x); } // Safe - Alternative 2 function withCheck2() { $x = mayReturnNull(); if ($x instanceof stdClass) { doesNotAcceptNull($x); } } Loading history...
240
241			$bridge->delete()
242			->by($this->entity->foreignKey, $this->id)
243			->run();
244
245			foreach ($ids as $id) {
246			$bridge->insert()
247			->data([
248			$this->entity->foreignKey => $this->id,
249			$entity->foreignKey => $id,
250			])
251			->run();
252			}
253
254			continue;
255			}
256			}
257
258			return $this;
259			}
260			}
261

oscarotero / simple-crud

Push — master ( 0fe8b6...bcfab5 )

Row::saveExternalRelations() C

Complexity

Size

Duplication

Importance

How to fix Long Method

Long Method

Available Fixes

Available Fixes

Available Fixes

Available Fixes

Duplication Side-by-Side

Filter issues like