TransferController - Code Metrics - Inspection of "[WIP] User actioned transfer of folders to other u..." - owncloud/core - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Pull Request — master (#32545)

by Tom

created 2018-09-02 14:49 UTC

TransferController A

↳ Parent: Project

Complexity

Total Complexity

Size/Duplication

Total Lines	105
Duplicated Lines	41.9 %

Coupling/Cohesion

Components	1
Dependencies	10

Importance

Changes

Metric	Value
dl	44
loc	105
rs	10
c	0
b	0
f	0
wmc	11
lcom	1
cbo	10

4 Methods

Rating	Name	Duplication	Size	Complexity
A	__construct()	0	13	1
A	transfer()	0	14	2
A	accept()	22	22	4
A	reject()	22	22	4

How to fix Duplicated Code

<?php
/**
 * @author Tom Needham <[email protected]>
 *
 * @copyright Copyright (c) 2018, ownCloud GmbH
 * @license AGPL-3.0
 *
 * This code is free software: you can redistribute it and/or modify
 * it under the terms of the GNU Affero General Public License, version 3,
 * as published by the Free Software Foundation.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 * GNU Affero General Public License for more details.
 *
 * You should have received a copy of the GNU Affero General Public License, version 3,
 * along with this program.  If not, see <http://www.gnu.org/licenses/>
 *
 */

namespace OCA\Files\Controller;

use OCA\Files\Service\TransferOwnership\TransferRequestManager;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\JSONResponse;
use OCP\ILogger;
use OCP\IRequest;
use OCP\IUserSession;

/**
 * Class TransferController
 *
 * @package OCA\Files\Controller
 */
class TransferController extends Controller {
	/** @var string */
	protected $appName;
	/** @var IRequest */
	protected $request;
	/** @var TransferRequestManager  */
	protected $transferManager;
	/** @var IUserSession  */
	protected $userSession;
	/** @var ILogger  */
	protected $logger;

	/**
	 * @param string $appName
	 * @param IRequest $request
	 * @param TransferRequestManager $transferManager
	 */
	public function __construct($appName,
								IRequest $request,
								TransferRequestManager $transferManager,
								IUserSession $userSession,
								ILogger $logger
	) {
		parent::__construct($appName, $request);
		$this->appName = $appName;
		$this->request = $request;
		$this->transferManager = $transferManager;
		$this->userSession = $userSession;
		$this->logger = $logger;
	}

	/**
	 * @NoAdminRequired
	 */
	public function transfer() {
		$file = $this->request->getParam('file');
		$dir = $this->request->getParam('dir');
		$destinationUser = $this->request->getParam('uid');
		$destinationUser = \OC::$server->getUserManager()->get($destinationUser);
		$sessionUser = \OC::$server->getUserSession()->getUser();
		$fileId = \OC::$server->getUserFolder($sessionUser->getUID())->get($dir.'/'.$file)->getId();
		try {
			$this->transferManager->newTransferRequest($sessionUser, $destinationUser, $fileId);
/** @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);
    }
}
			return new JSONResponse();
		} catch (\Exception $e) {
			return new JSONResponse(['message' => $e->getMessage()], Http::STATUS_INTERNAL_SERVER_ERROR);
		}
	}

	/**
	 * @NoAdminRequired
	 */
	public function accept($requestId) {
		// Check this user is the recipient of this request
		try {
			// Not found, bad request
			$request = $this->transferManager->getRequestById($requestId);
		} catch (DoesNotExistException $e) {
			return new JSONResponse([], Http::STATUS_BAD_REQUEST);
		}
		$sessionUser = $this->userSession->getUser();
		if ($request->getDestinationUserId() !== $sessionUser->getUID()) {
class ParentClass {
    private $data = array();

    public function __call($method, array $args) {
        if (0 === strpos($method, 'get')) {
            return $this->data[strtolower(substr($method, 3))];
        }

        throw new \LogicException(sprintf('Unsupported method: %s', $method));
    }
}

/**
 * If this class knows which fields exist, you can specify the methods here:
 *
 * @method string getName()
 */
class SomeClass extends ParentClass { }
			// This isn't their request to respond to, bad request
			return new JSONResponse([], Http::STATUS_BAD_REQUEST);
		}

		try {
			$this->transferManager->acceptRequest($request);

			return new JSONResponse([]);
		} catch (\Exception $e) {
			$this->logger->logException($e, ['app' => 'files']);
			return new JSONResponse([], Http::STATUS_INTERNAL_SERVER_ERROR);
		}
	}

	/**
	 * @NoAdminRequired
	 */
	public function reject($requestId) {
		// Check this user is the recipient of this request
		try {
			// Not found, bad request
			$request = $this->transferManager->getRequestById($requestId);
		} catch (DoesNotExistException $e) {
			return new JSONResponse([], Http::STATUS_BAD_REQUEST);
		}
		$sessionUser = $this->userSession->getUser();
		if ($request->getDestinationUserId() !== $sessionUser->getUID()) {
class ParentClass {
    private $data = array();

    public function __call($method, array $args) {
        if (0 === strpos($method, 'get')) {
            return $this->data[strtolower(substr($method, 3))];
        }

        throw new \LogicException(sprintf('Unsupported method: %s', $method));
    }
}

/**
 * If this class knows which fields exist, you can specify the methods here:
 *
 * @method string getName()
 */
class SomeClass extends ParentClass { }
			// This isn't their request to respond to, bad request
			return new JSONResponse([], Http::STATUS_BAD_REQUEST);
		}

		try {
			$this->transferManager->rejectRequest($request);

			return new JSONResponse([]);
		} catch (\Exception $e) {
			$this->logger->logException($e, ['app' => 'files']);
			return new JSONResponse([], Http::STATUS_INTERNAL_SERVER_ERROR);
		}
	}




}


1		<?php
2		/**
3		* @author Tom Needham <[email protected]>
4		*
5		* @copyright Copyright (c) 2018, ownCloud GmbH
6		* @license AGPL-3.0
7		*
8		* This code is free software: you can redistribute it and/or modify
9		* it under the terms of the GNU Affero General Public License, version 3,
10		* as published by the Free Software Foundation.
11		*
12		* This program is distributed in the hope that it will be useful,
13		* but WITHOUT ANY WARRANTY; without even the implied warranty of
14		* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15		* GNU Affero General Public License for more details.
16		*
17		* You should have received a copy of the GNU Affero General Public License, version 3,
18		* along with this program. If not, see <http://www.gnu.org/licenses/>
19		*
20		*/
21
22		namespace OCA\Files\Controller;
23
24		use OCA\Files\Service\TransferOwnership\TransferRequestManager;
25		use OCP\AppFramework\Controller;
26		use OCP\AppFramework\Db\DoesNotExistException;
27		use OCP\AppFramework\Http;
28		use OCP\AppFramework\Http\JSONResponse;
29		use OCP\ILogger;
30		use OCP\IRequest;
31		use OCP\IUserSession;
32
33		/**
34		* Class TransferController
35		*
36		* @package OCA\Files\Controller
37		*/
38		class TransferController extends Controller {
39		/** @var string */
40		protected $appName;
41		/** @var IRequest */
42		protected $request;
43		/** @var TransferRequestManager */
44		protected $transferManager;
45		/** @var IUserSession */
46		protected $userSession;
47		/** @var ILogger */
48		protected $logger;
49
50		/**
51		* @param string $appName
52		* @param IRequest $request
53		* @param TransferRequestManager $transferManager
54		*/
55		public function __construct($appName,
56		IRequest $request,
57		TransferRequestManager $transferManager,
58		IUserSession $userSession,
59		ILogger $logger
60		) {
61		parent::__construct($appName, $request);
62		$this->appName = $appName;
63		$this->request = $request;
64		$this->transferManager = $transferManager;
65		$this->userSession = $userSession;
66		$this->logger = $logger;
67		}
68
69		/**
70		* @NoAdminRequired
71		*/
72		public function transfer() {
73		$file = $this->request->getParam('file');
74		$dir = $this->request->getParam('dir');
75		$destinationUser = $this->request->getParam('uid');
76		$destinationUser = \OC::$server->getUserManager()->get($destinationUser);
77		$sessionUser = \OC::$server->getUserSession()->getUser();
78		$fileId = \OC::$server->getUserFolder($sessionUser->getUID())->get($dir.'/'.$file)->getId();
79		try {
80		$this->transferManager->newTransferRequest($sessionUser, $destinationUser, $fileId);
		0 ignored issues – show Bug introduced 2018-09-01 23:43 UTC by Report Bug Copy Issue Report It seems like `$sessionUser` defined by `\OC::$server->getUserSession()->getUser()` on line `77` can be `null`; however, `OCA\Files\Service\Transf...r::newTransferRequest()` 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... Bug introduced 2018-09-01 23:43 UTC by Report Bug Copy Issue Report It seems like `$destinationUser` defined by `\OC::$server->getUserMan...->get($destinationUser)` on line `76` can be `null`; however, `OCA\Files\Service\Transf...r::newTransferRequest()` 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...
81		return new JSONResponse();
82		} catch (\Exception $e) {
83		return new JSONResponse(['message' => $e->getMessage()], Http::STATUS_INTERNAL_SERVER_ERROR);
84		}
85		}
86
87		/**
88		* @NoAdminRequired
89		*/
90	View Code Duplication	public function accept($requestId) {
91		// Check this user is the recipient of this request
92		try {
93		// Not found, bad request
94		$request = $this->transferManager->getRequestById($requestId);
95		} catch (DoesNotExistException $e) {
96		return new JSONResponse([], Http::STATUS_BAD_REQUEST);
97		}
98		$sessionUser = $this->userSession->getUser();
99		if ($request->getDestinationUserId() !== $sessionUser->getUID()) {
		0 ignored issues – show Documentation Bug introduced 2018-09-01 23:43 UTC by Report Bug Copy Issue Report The method `getDestinationUserId` does not exist on `object<OCP\AppFramework\Db\Entity>`? Since you implemented `__call`, maybe consider adding a @method annotation. If you implement `__call` and you know which methods are available, you can improve IDE auto-completion and static analysis by adding a @method annotation to the class. This is often the case, when `__call` is implemented by a parent class and only the child class knows which methods exist: class ParentClass { private $data = array(); public function __call($method, array $args) { if (0 === strpos($method, 'get')) { return $this->data[strtolower(substr($method, 3))]; } throw new \LogicException(sprintf('Unsupported method: %s', $method)); } } /** * If this class knows which fields exist, you can specify the methods here: * * @method string getName() */ class SomeClass extends ParentClass { } Loading history...
100		// This isn't their request to respond to, bad request
101		return new JSONResponse([], Http::STATUS_BAD_REQUEST);
102		}
103
104		try {
105		$this->transferManager->acceptRequest($request);
		0 ignored issues – show Compatibility introduced 2018-09-01 23:43 UTC by Report Bug Copy Issue Report `$request` of type `object<OCP\AppFramework\Db\Entity>` is not a sub-type of `object<OCA\Files\Service...ership\TransferRequest>`. It seems like you assume a child class of the class `OCP\AppFramework\Db\Entity` to be always present. This check looks for parameters that are defined as one type in their type hint or doc comment but seem to be used as a narrower type, i.e an implementation of an interface or a subclass. Consider changing the type of the parameter or doing an instanceof check before assuming your parameter is of the expected type. Loading history...
106		return new JSONResponse([]);
107		} catch (\Exception $e) {
108		$this->logger->logException($e, ['app' => 'files']);
109		return new JSONResponse([], Http::STATUS_INTERNAL_SERVER_ERROR);
110		}
111		}
112
113		/**
114		* @NoAdminRequired
115		*/
116	View Code Duplication	public function reject($requestId) {
117		// Check this user is the recipient of this request
118		try {
119		// Not found, bad request
120		$request = $this->transferManager->getRequestById($requestId);
121		} catch (DoesNotExistException $e) {
122		return new JSONResponse([], Http::STATUS_BAD_REQUEST);
123		}
124		$sessionUser = $this->userSession->getUser();
125		if ($request->getDestinationUserId() !== $sessionUser->getUID()) {
		0 ignored issues – show Documentation Bug introduced 2018-09-01 23:43 UTC by Report Bug Copy Issue Report The method `getDestinationUserId` does not exist on `object<OCP\AppFramework\Db\Entity>`? Since you implemented `__call`, maybe consider adding a @method annotation. If you implement `__call` and you know which methods are available, you can improve IDE auto-completion and static analysis by adding a @method annotation to the class. This is often the case, when `__call` is implemented by a parent class and only the child class knows which methods exist: class ParentClass { private $data = array(); public function __call($method, array $args) { if (0 === strpos($method, 'get')) { return $this->data[strtolower(substr($method, 3))]; } throw new \LogicException(sprintf('Unsupported method: %s', $method)); } } /** * If this class knows which fields exist, you can specify the methods here: * * @method string getName() */ class SomeClass extends ParentClass { } Loading history...
126		// This isn't their request to respond to, bad request
127		return new JSONResponse([], Http::STATUS_BAD_REQUEST);
128		}
129
130		try {
131		$this->transferManager->rejectRequest($request);
		0 ignored issues – show Compatibility introduced 2018-09-01 23:43 UTC by Report Bug Copy Issue Report `$request` of type `object<OCP\AppFramework\Db\Entity>` is not a sub-type of `object<OCA\Files\Service...ership\TransferRequest>`. It seems like you assume a child class of the class `OCP\AppFramework\Db\Entity` to be always present. This check looks for parameters that are defined as one type in their type hint or doc comment but seem to be used as a narrower type, i.e an implementation of an interface or a subclass. Consider changing the type of the parameter or doing an instanceof check before assuming your parameter is of the expected type. Loading history...
132		return new JSONResponse([]);
133		} catch (\Exception $e) {
134		$this->logger->logException($e, ['app' => 'files']);
135		return new JSONResponse([], Http::STATUS_INTERNAL_SERVER_ERROR);
136		}
137		}
138
139
140
141
142		}
143

owncloud / core

Pull Request — master (#32545)

TransferController A

Complexity

Size/Duplication

Coupling/Cohesion

Importance

4 Methods

How to fix Duplicated Code

Duplicated Code

Duplication Side-by-Side

Filter issues like