From 1169c5c5e295cad8ce35d623dbd4375f3c7c4d06 Mon Sep 17 00:00:00 2001 From: James Cole Date: Sat, 1 Aug 2020 17:51:52 +0200 Subject: [PATCH] Various code quality improvements. --- .ci/phpstan.neon | 1 + .../Upgrade/OtherCurrenciesCorrections.php | 2 +- .../Upgrade/TransferCurrenciesCorrections.php | 4 +-- app/Factory/BillFactory.php | 14 ++++----- app/Factory/TransactionJournalFactory.php | 30 +++++++++---------- database/factories/EmptyFactory.php | 6 ++-- routes/breadcrumbs.php | 1 + 7 files changed, 30 insertions(+), 28 deletions(-) diff --git a/.ci/phpstan.neon b/.ci/phpstan.neon index 4cdb120e5a..f32333ea46 100644 --- a/.ci/phpstan.neon +++ b/.ci/phpstan.neon @@ -8,6 +8,7 @@ parameters: ignoreErrors: - '#is not allowed to extend#' - '#is neither abstract nor final#' + - '#Control structures using switch should not be used\.#' paths: - ../app - ../database diff --git a/app/Console/Commands/Upgrade/OtherCurrenciesCorrections.php b/app/Console/Commands/Upgrade/OtherCurrenciesCorrections.php index c2f082b784..cefd9bca1b 100644 --- a/app/Console/Commands/Upgrade/OtherCurrenciesCorrections.php +++ b/app/Console/Commands/Upgrade/OtherCurrenciesCorrections.php @@ -101,7 +101,7 @@ class OtherCurrenciesCorrections extends Command if (array_key_exists($accountId, $this->accountCurrencies) && 0 === $this->accountCurrencies[$accountId]) { return null; // @codeCoverageIgnore } - if (isset($this->accountCurrencies[$accountId]) && $this->accountCurrencies[$accountId] instanceof TransactionCurrency) { + if (array_key_exists($accountId, $this->accountCurrencies) && $this->accountCurrencies[$accountId] instanceof TransactionCurrency) { return $this->accountCurrencies[$accountId]; // @codeCoverageIgnore } $currency = $this->accountRepos->getAccountCurrency($account); diff --git a/app/Console/Commands/Upgrade/TransferCurrenciesCorrections.php b/app/Console/Commands/Upgrade/TransferCurrenciesCorrections.php index d8a9e4eff7..752bac5087 100644 --- a/app/Console/Commands/Upgrade/TransferCurrenciesCorrections.php +++ b/app/Console/Commands/Upgrade/TransferCurrenciesCorrections.php @@ -335,10 +335,10 @@ class TransferCurrenciesCorrections extends Command private function getCurrency(Account $account): ?TransactionCurrency { $accountId = $account->id; - if (isset($this->accountCurrencies[$accountId]) && 0 === $this->accountCurrencies[$accountId]) { + if (array_key_exists($accountId, $this->accountCurrencies) && 0 === $this->accountCurrencies[$accountId]) { return null; // @codeCoverageIgnore } - if (isset($this->accountCurrencies[$accountId]) && $this->accountCurrencies[$accountId] instanceof TransactionCurrency) { + if (array_key_exists($accountId, $this->accountCurrencies) && $this->accountCurrencies[$accountId] instanceof TransactionCurrency) { return $this->accountCurrencies[$accountId]; // @codeCoverageIgnore } $currency = $this->accountRepos->getAccountCurrency($account); diff --git a/app/Factory/BillFactory.php b/app/Factory/BillFactory.php index 038b601911..b4dfc6b4b7 100644 --- a/app/Factory/BillFactory.php +++ b/app/Factory/BillFactory.php @@ -59,15 +59,15 @@ class BillFactory /** * @param array $data * - * @throws FireflyException * @return Bill|null + * @throws FireflyException */ public function create(array $data): ?Bill { /** @var TransactionCurrencyFactory $factory */ $factory = app(TransactionCurrencyFactory::class); /** @var TransactionCurrency $currency */ - $currency = $factory->find((int) ($data['currency_id'] ?? null), (string) ($data['currency_code'] ?? null)); + $currency = $factory->find((int)($data['currency_id'] ?? null), (string)($data['currency_code'] ?? null)); if (null === $currency) { $currency = app('amount')->getDefaultCurrencyByUser($this->user); @@ -95,8 +95,8 @@ class BillFactory throw new FireflyException('400000: Could not store bill.'); } - if (isset($data['notes'])) { - $this->updateNote($bill, $data['notes']); + if (array_key_exists('notes', $data)) { + $this->updateNote($bill, (string)$data['notes']); } $objectGroupTitle = $data['object_group'] ?? ''; @@ -108,7 +108,7 @@ class BillFactory } } // try also with ID: - $objectGroupId = (int) ($data['object_group_id'] ?? 0); + $objectGroupId = (int)($data['object_group_id'] ?? 0); if (0 !== $objectGroupId) { $objectGroup = $this->findObjectGroupById($objectGroupId); if (null !== $objectGroup) { @@ -128,8 +128,8 @@ class BillFactory */ public function find(?int $billId, ?string $billName): ?Bill { - $billId = (int) $billId; - $billName = (string) $billName; + $billId = (int)$billId; + $billName = (string)$billName; $bill = null; // first find by ID: if ($billId > 0) { diff --git a/app/Factory/TransactionJournalFactory.php b/app/Factory/TransactionJournalFactory.php index 42dc920e26..4214bddd96 100644 --- a/app/Factory/TransactionJournalFactory.php +++ b/app/Factory/TransactionJournalFactory.php @@ -57,8 +57,8 @@ class TransactionJournalFactory use JournalServiceTrait; private AccountRepositoryInterface $accountRepository; - private AccountValidator $accountValidator; - private BillRepositoryInterface $billRepository; + private AccountValidator $accountValidator; + private BillRepositoryInterface $billRepository; /** @var CurrencyRepositoryInterface */ private $currencyRepository; /** @var bool */ @@ -87,7 +87,7 @@ class TransactionJournalFactory { $this->errorOnHash = false; // TODO move valid meta fields to config. - $this->fields = [ + $this->fields = [ // sepa 'sepa_cc', 'sepa_ct_op', 'sepa_ct_id', 'sepa_db', 'sepa_country', 'sepa_ep', @@ -128,9 +128,9 @@ class TransactionJournalFactory * * @param array $data * - * @throws DuplicateTransactionException - * @throws FireflyException * @return Collection + * @throws FireflyException + * @throws DuplicateTransactionException */ public function create(array $data): Collection { @@ -246,9 +246,9 @@ class TransactionJournalFactory /** * @param NullArrayObject $row * - * @throws FireflyException - * @throws DuplicateTransactionException * @return TransactionJournal|null + * @throws DuplicateTransactionException + * @throws FireflyException */ private function createJournal(NullArrayObject $row): ?TransactionJournal { @@ -306,10 +306,10 @@ class TransactionJournalFactory Log::debug('Now calling getAccount for the destination.'); $destinationAccount = $this->getAccount($type->type, 'destination', $destInfo); Log::debug('Done with getAccount(2x)'); - $currency = $this->getCurrencyByAccount($type->type, $currency, $sourceAccount, $destinationAccount); - $foreignCurrency = $this->compareCurrencies($currency, $foreignCurrency); - $foreignCurrency = $this->getForeignByAccount($type->type, $foreignCurrency, $destinationAccount); - $description = $this->getDescription($description); + $currency = $this->getCurrencyByAccount($type->type, $currency, $sourceAccount, $destinationAccount); + $foreignCurrency = $this->compareCurrencies($currency, $foreignCurrency); + $foreignCurrency = $this->getForeignByAccount($type->type, $foreignCurrency, $destinationAccount); + $description = $this->getDescription($description); /** Create a basic journal. */ $journal = TransactionJournal::create( @@ -596,8 +596,8 @@ class TransactionJournalFactory $this->accountValidator->setTransactionType($transactionType); // validate source account. - $sourceId = isset($data['source_id']) ? (int) $data['source_id'] : null; - $sourceName = isset($data['source_name']) ? (string) $data['source_name'] : null; + $sourceId = array_key_exists('source_id', $data) ? (int) $data['source_id'] : null; + $sourceName = array_key_exists('source_name', $data) ? (string) $data['source_name'] : null; $validSource = $this->accountValidator->validateSource($sourceId, $sourceName, null); // do something with result: @@ -606,8 +606,8 @@ class TransactionJournalFactory } Log::debug('Source seems valid.'); // validate destination account - $destinationId = isset($data['destination_id']) ? (int) $data['destination_id'] : null; - $destinationName = isset($data['destination_name']) ? (string) $data['destination_name'] : null; + $destinationId = array_key_exists('destination_id', $data) ? (int) $data['destination_id'] : null; + $destinationName = array_key_exists('destination_name', $data) ? (string) $data['destination_name'] : null; $validDestination = $this->accountValidator->validateDestination($destinationId, $destinationName, null); // do something with result: if (false === $validDestination) { diff --git a/database/factories/EmptyFactory.php b/database/factories/EmptyFactory.php index 1f65f7e821..0c4adc7fc9 100644 --- a/database/factories/EmptyFactory.php +++ b/database/factories/EmptyFactory.php @@ -1,11 +1,11 @@ define(Model::class, function (Faker $faker) { +$factory->define(User::class, function (Faker $faker) { return [ // ]; diff --git a/routes/breadcrumbs.php b/routes/breadcrumbs.php index 76cbc0ffd3..068bce92b9 100644 --- a/routes/breadcrumbs.php +++ b/routes/breadcrumbs.php @@ -1198,4 +1198,5 @@ try { ); } catch (DuplicateBreadcrumbException $e) { + // @ignoreException }