refactor: enhance input validation, console traits and repository methods - #41
Conversation
- Add getValidatedOptionOrPrompt to ConsoleInputTrait for seamless option/prompt validation - Update ServerValidationTrait with improved validation methods - Enhance ServerAddCommand with new validation patterns - Add comprehensive tests for validation traits and input handling - Update documentation in rules for new patterns
- Enhance ServerRepository with better data handling - Update corresponding unit tests for new repository behavior - Fix validation trait updates in tests
- Include remaining test updates for ServerRepository - Ensure full test coverage for recent refactoring changes
|
Caution Review failedThe pull request is closed. WalkthroughAdds getValidatedOptionOrPrompt for validated interactive prompts, applies it in ServerAddCommand, adds findByHost and host-duplication checks to ServerRepository, converts validators to return error strings, tweaks ServerHelpersTrait signature, and updates tests to assert exit codes/output and cover new validation flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Cmd as ServerAddCommand
participant CIT as ConsoleInputTrait
participant Prompt as Prompter
participant Val as ServerValidationTrait
participant Repo as ServerRepository
User->>Cmd: run add server
Cmd->>CIT: getValidatedOptionOrPrompt(option, promptCb, validator)
alt CLI option provided
CIT->>Val: validator(optionValue)
alt valid
CIT-->>Cmd: value
else invalid
CIT-->>Cmd: null (emit error)
end
else no CLI option
CIT->>Prompt: promptCb(validator)
Prompt-->>CIT: userInput (validated interactively)
CIT-->>Cmd: userInput
end
Cmd->>Val: validateNameInput(name)
alt name invalid
Cmd-->>User: FAILURE (message)
else
Cmd->>Val: validateHostInput(host)
alt host invalid
Cmd-->>User: FAILURE (message)
else
Cmd->>Val: validatePortInput(port)
alt port invalid
Cmd-->>User: FAILURE (message)
else
Cmd->>Repo: findByHost(host)
alt host exists
Repo-->>Cmd: existing ServerDTO
Cmd-->>User: FAILURE (host already used)
else
Cmd->>Repo: create(...)
Repo-->>Cmd: ServerDTO
Cmd-->>User: SUCCESS (details)
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
tests/Fixtures/TestConsoleCommand.php (1)
178-209: Silence unused-parameter warning in invalid validatorRename the unused parameter to signal intent and appease PHPMD.
Based on static analysis hints
- fn ($value) => 'Always invalid' + fn ($_value) => 'Always invalid'app/Repositories/ServerRepository.php (1)
52-61: Host uniqueness check added; confirm desired uniqueness scope and import RuntimeException
- Behavior: disallows duplicate hosts regardless of port. Is the intent to uniquely constrain by host only, or host+port? Please confirm.
- Style: avoid FQCN; import RuntimeException and use it directly.
As per coding guidelines
Apply within this range:
- if (null !== $existingName) { - throw new \RuntimeException("Server '{$server->name}' already exists"); + if (null !== $existingName) { + throw new RuntimeException("Server '{$server->name}' already exists"); } @@ - if (null !== $existingHost) { - throw new \RuntimeException("Host '{$server->host}' is already used by server '{$existingHost->name}'"); + if (null !== $existingHost) { + throw new RuntimeException("Host '{$server->host}' is already used by server '{$existingHost->name}'"); }And add import at the top (outside this range):
use RuntimeException;app/Traits/ServerHelpersTrait.php (1)
59-66: Use repository lookup instead of manual loopReplace the loop with servers->findByName() for clarity and reuse.
- $server = null; - foreach ($allServers as $s) { - if ($s->name === $name) { - $server = $s; - break; - } - } + $server = $this->servers->findByName($name);app/Traits/ServerValidationTrait.php (2)
42-63: Host validation: consider localhost and case normalization
- format: FILTER_VALIDATE_DOMAIN(FILTER_FLAG_HOSTNAME) rejects 'localhost'. Do we want to accept 'localhost'?
- uniqueness: should host comparisons be case-insensitive for domains?
If desired, you can allow localhost cheaply:
- $isValidIp = filter_var($host, FILTER_VALIDATE_IP) !== false; - $isValidDomain = filter_var($host, FILTER_VALIDATE_DOMAIN, FILTER_FLAG_HOSTNAME) !== false; + $isValidIp = filter_var($host, FILTER_VALIDATE_IP) !== false; + $isValidDomain = filter_var($host, FILTER_VALIDATE_DOMAIN, FILTER_FLAG_HOSTNAME) !== false; + $isLocalhost = strtolower($host) === 'localhost'; @@ - if (!$isValidIp && !$isValidDomain) { + if (!$isValidIp && !$isValidDomain && !$isLocalhost) { return 'Must be a valid IP address or domain name (e.g., 192.168.1.100, example.com)'; }
70-86: Accept int or string for port; use union type and simpler checksPrompts return strings, but options/callers may pass ints. Support both and keep messages stable.
As per coding guidelines
- protected function validatePortInput(mixed $portString): ?string + protected function validatePortInput(string|int $port): ?string { - if (!is_string($portString)) { - return 'Port must be a string'; - } - - if (!ctype_digit($portString)) { - return 'Port must be a number'; - } - - $port = (int) $portString; + if (is_string($port)) { + if (!ctype_digit($port)) { + return 'Port must be a number'; + } + $port = (int) $port; + } if ($port < 1 || $port > 65535) { return 'Port must be between 1 and 65535 (common SSH ports: 22, 2222, 22000)'; } return null; }tests/Unit/Traits/ConsoleInputTraitTest.php (1)
188-203: Clarify intent of “non-interactive” wording (optional)This test exercises the prompt path; “non-interactive mode” could be misread. Consider renaming to “validator is passed to prompt callback when prompting,” or extend MockPrompter to assert the validate callable was received.
.cursor/rules/03-commands.mdc (1)
298-436: Great validation pattern; add two clarifications
- Note that getValidatedOptionOrPrompt is intended for VALUE_REQUIRED data options, not boolean flags.
- Add a brief generics note: it returns T|null, where T is the prompt’s return type.
Example insert:
+Note: +- Use getValidatedOptionOrPrompt only with VALUE_REQUIRED data options (e.g., --name, --host, --port). +- For boolean flags (VALUE_NONE), continue using getOptionOrPrompt. + +Types: +- Conceptually returns T|null where T is the prompt’s return type (string/int/etc.). Document this in phpdoc for better static analysis.app/Traits/ConsoleInputTrait.php (1)
101-149: Logic LGTM; document scope and add phpdoc generics
- Intended for VALUE_REQUIRED options; avoid using on boolean flags (VALUE_NONE). Add this to phpdoc.
- Improve phpdoc typing to mirror getOptionOrPrompt: declare template T and return T|null for better static analysis.
Suggested phpdoc tweak:
- * @param Closure(Closure): mixed $promptCallback ... - * @return mixed The validated value, or null if validation failed + * @template T + * @param Closure(callable(mixed): (?string)): T $promptCallback Closure receiving the validator and returning T + * @param Closure(mixed): (?string) $validator Validation closure that returns error or null + * @return T|null The validated value, or null if validation failed + * + * Note: Use only with VALUE_REQUIRED options (data inputs), not boolean flags.As per coding guidelines
tests/Integration/Console/Server/ServerAddCommandTest.php (1)
179-197: Invalid port scenario covered; consider more robust regexPattern '/Port must be|between 1 and 65535/' works but could be clearer:
- ->toMatch('/Port must be|between 1 and 65535/') + ->toMatch('/(Port must be|between 1 and 65535)/') + // or assert the full canonical message if stable: + // ->toContain('Port must be between 1 and 65535')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.cursor/rules/03-commands.mdc(2 hunks)app/Console/Server/ServerAddCommand.php(1 hunks)app/Repositories/ServerRepository.php(2 hunks)app/Traits/ConsoleInputTrait.php(1 hunks)app/Traits/ServerHelpersTrait.php(2 hunks)app/Traits/ServerValidationTrait.php(1 hunks)tests/Fixtures/TestConsoleCommand.php(2 hunks)tests/Integration/Console/Server/ServerAddCommandTest.php(3 hunks)tests/Unit/Repositories/ServerRepositoryTest.php(2 hunks)tests/Unit/Traits/ConsoleInputTraitTest.php(1 hunks)tests/Unit/Traits/ServerValidationTraitTest.php(3 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
**/*.php: Eliminate single-use private methods by inlining them directly
Cache expensive computed values by initializing them in the constructor instead of recomputing
Prefer direct property access over method calls when appropriate to avoid call overhead
Organize code into comment-separated sections; prefer alphabetical ordering when it does not conflict with logical grouping
**/*.php: Adhere to PSR-12 coding style in all PHP files
Declare strict_types=1 at the top of every PHP file
Prefer PHP 8.x features (union types, match, attributes, readonly) where appropriate
Always import classes with use statements; avoid fully qualified class names in code bodies
All methods must declare explicit return types; use proper generics in types/docblocks (e.g., Collection<int, User>)
Use dependency injection instead of manually resolving or instantiating classes
Prefer Symfony component classes (e.g., Filesystem, Process) over native PHP functions for testability
All object creation must use $container->build(ClassName::class) instead of new, except for value objects/DTOs/pure data structures
In production code, access the Container via constructor injection, not via static/global access
Add minimal DocBlock comments with descriptions, parameters, and return types for classes and functions
Use comments as visual separators for sections/subsections with a single newline between header, subheader, and paragraph; avoid obvious or stale comments
Run rector on changed PHP files before completing a task
Run pint on changed PHP files to fix code style before completing a task
Files:
app/Repositories/ServerRepository.phpapp/Traits/ConsoleInputTrait.phpapp/Console/Server/ServerAddCommand.phptests/Unit/Repositories/ServerRepositoryTest.phpapp/Traits/ServerValidationTrait.phptests/Integration/Console/Server/ServerAddCommandTest.phptests/Unit/Traits/ConsoleInputTraitTest.phpapp/Traits/ServerHelpersTrait.phptests/Fixtures/TestConsoleCommand.phptests/Unit/Traits/ServerValidationTraitTest.php
app/{Contracts/BaseCommand.php,Traits/ConsoleOutputTrait.php,Traits/ConsoleInputTrait.php}
📄 CodeRabbit inference engine (.cursor/rules/03-commands.mdc)
If output functionality is missing, add a new method to BaseCommand (and the appropriate Console*Trait) with modern styling and documentation
Files:
app/Traits/ConsoleInputTrait.php
app/{Console/**,Traits/ConsoleInputTrait.php}
📄 CodeRabbit inference engine (.cursor/rules/03-commands.mdc)
Use laravel/prompts for all user interactions (text, password, confirm, select, multiselect, suggest, search, spin)
Files:
app/Traits/ConsoleInputTrait.phpapp/Console/Server/ServerAddCommand.php
app/Traits/ConsoleInputTrait.php
📄 CodeRabbit inference engine (.cursor/rules/03-commands.mdc)
Add input gathering methods to ConsoleInputTrait; methods should work with $this->input and wrap Laravel Prompts
Files:
app/Traits/ConsoleInputTrait.php
**/*Command.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
**/*Command.php: Commands handle user interaction (input/output) and orchestrate services
Commands must not contain business logic; delegate business logic to Services
Commands must not duplicate orchestration logic; extract shared orchestration to Services
Commands should not invoke other commands (no proxy commands)
Use SymfonyStyle consistently for all user-facing console output
Files:
app/Console/Server/ServerAddCommand.phptests/Fixtures/TestConsoleCommand.php
app/Console/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/03-commands.mdc)
app/Console/**/*.php: Never use Symfony IO methods directly in console commands; use BaseCommand custom IO methods (writeln, hr, h1, info, success, error, warning) exclusively
Use status helper methods (success, error, warning, info) for all status messages to ensure consistent formatting
Commands handle all user interaction (input/output) and format output; validation errors bubble up to commands for display
Support both interactive prompts and CLI options using getOptionOrPrompt(optionName, promptCallback) for dual-mode commands
Use only options, never arguments, in commands (to enable getOptionOrPrompt)
Pair every option with getOptionOrPrompt to provide interactive fallbacks
Boolean flags must use InputOption::VALUE_NONE
Data input options must use InputOption::VALUE_REQUIRED
Only the --yes option gets a short flag (-y)
Use standardized option names: --server, --site, --name, --host, --port, --yes/-y, --skip per their specified types and usages
Always call showCommandHint() before returning Command::SUCCESS to teach non-interactive usage
Do not access $this->io->writeln/text/success/etc. directly in commands; route all output through BaseCommand helpers
Files:
app/Console/Server/ServerAddCommand.php
{tests/**,test/**,**/*@(Test|Spec).php}
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Do not run or edit tests unless explicitly instructed
Files:
tests/Unit/Repositories/ServerRepositoryTest.phptests/Integration/Console/Server/ServerAddCommandTest.phptests/Unit/Traits/ConsoleInputTraitTest.phptests/Fixtures/TestConsoleCommand.phptests/Unit/Traits/ServerValidationTraitTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
tests/**/*.php: In tests, direct Container instantiation and bind() for mocks is allowed and encouraged for isolation
Do not run PHPStan on test files; tests are excluded from static analysis
tests/**/*.php: Unit tests must instantiate services manually (no DI container)
Command/integration tests must use mockCommandContainer() for building commands and overriding services
Only use container auto-wiring in tests to verify DI configuration or multi-service integration (edge cases)
Keep test files under 1.8x the size of the source they test (without sacrificing readability)
Test core business logic; avoid testing the framework itself
Prefer dataset-driven testing using ->with([...]) for multiple scenarios
Consolidate related assertions (e.g., expect($x)->toBe(...)->and($y)->toBe(...))
Mock only external dependencies; keep unit tests isolated from filesystem/HTTP/processes
Avoid performance tests unless performance is the primary concern
Use the AAA pattern in tests (Arrange, Act, Assert; optional Cleanup)
In exception tests, use a combined // ACT & ASSERT step when the act triggers the assertion
Organize tests with describe() blocks, beforeEach() setup, and shared helpers/traits for DRY
Forbidden assertions in tests: type-only or generic checks (e.g., toBeInstanceOf, toBeArray, not->toBeNull, expect(true)->toBeTrue) and sleep(...); prefer time mocking
Preferred assertions: assert observable behavior and interactions (e.g., domain values, validator outcomes, mock expectations)
Unit tests: mock all external dependencies, test single units in isolation, and complete in milliseconds
Integration tests: use real file operations and external processes; cover CLI commands and full workflows
Do not require PHPStan compliance in tests; avoid excessive phpdoc solely to satisfy types in tests
Files:
tests/Unit/Repositories/ServerRepositoryTest.phptests/Integration/Console/Server/ServerAddCommandTest.phptests/Unit/Traits/ConsoleInputTraitTest.phptests/Fixtures/TestConsoleCommand.phptests/Unit/Traits/ServerValidationTraitTest.php
🧠 Learnings (7)
📚 Learning: 2025-10-11T15:19:19.495Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/03-commands.mdc:0-0
Timestamp: 2025-10-11T15:19:19.495Z
Learning: Applies to app/Console/**/*.php : Support both interactive prompts and CLI options using getOptionOrPrompt(optionName, promptCallback) for dual-mode commands
Applied to files:
app/Traits/ConsoleInputTrait.phptests/Unit/Traits/ConsoleInputTraitTest.php
📚 Learning: 2025-10-11T15:19:19.496Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/03-commands.mdc:0-0
Timestamp: 2025-10-11T15:19:19.496Z
Learning: Applies to app/Traits/ConsoleInputTrait.php : Add input gathering methods to ConsoleInputTrait; methods should work with $this->input and wrap Laravel Prompts
Applied to files:
app/Traits/ConsoleInputTrait.php.cursor/rules/03-commands.mdc
📚 Learning: 2025-10-11T15:19:19.495Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/03-commands.mdc:0-0
Timestamp: 2025-10-11T15:19:19.495Z
Learning: Applies to app/Console/**/*.php : Pair every option with getOptionOrPrompt to provide interactive fallbacks
Applied to files:
app/Traits/ConsoleInputTrait.phptests/Unit/Traits/ConsoleInputTraitTest.php.cursor/rules/03-commands.mdc
📚 Learning: 2025-10-11T15:19:19.495Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/03-commands.mdc:0-0
Timestamp: 2025-10-11T15:19:19.495Z
Learning: Applies to app/{Console/**,Traits/ConsoleInputTrait.php} : Use laravel/prompts for all user interactions (text, password, confirm, select, multiselect, suggest, search, spin)
Applied to files:
app/Traits/ConsoleInputTrait.php.cursor/rules/03-commands.mdc
📚 Learning: 2025-10-11T15:19:19.495Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/03-commands.mdc:0-0
Timestamp: 2025-10-11T15:19:19.495Z
Learning: Applies to app/{Contracts/BaseCommand.php,Traits/ConsoleOutputTrait.php,Traits/ConsoleInputTrait.php} : If output functionality is missing, add a new method to BaseCommand (and the appropriate Console*Trait) with modern styling and documentation
Applied to files:
.cursor/rules/03-commands.mdc
📚 Learning: 2025-10-11T15:19:19.496Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/03-commands.mdc:0-0
Timestamp: 2025-10-11T15:19:19.496Z
Learning: Applies to app/Traits/ConsoleOutputTrait.php : Add output/formatting methods to ConsoleOutputTrait; methods should work with $this->io (SymfonyStyle)
Applied to files:
.cursor/rules/03-commands.mdc
📚 Learning: 2025-10-11T15:19:19.495Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/03-commands.mdc:0-0
Timestamp: 2025-10-11T15:19:19.495Z
Learning: Applies to app/Console/**/*.php : Use status helper methods (success, error, warning, info) for all status messages to ensure consistent formatting
Applied to files:
.cursor/rules/03-commands.mdc
🧬 Code graph analysis (10)
app/Repositories/ServerRepository.php (1)
app/DTOs/ServerDTO.php (1)
ServerDTO(7-17)
app/Traits/ConsoleInputTrait.php (1)
app/Traits/ConsoleOutputTrait.php (1)
error(62-65)
app/Console/Server/ServerAddCommand.php (2)
app/Traits/ConsoleInputTrait.php (2)
getValidatedOptionOrPrompt(126-148)promptText(166-182)app/Traits/ServerValidationTrait.php (3)
validateNameInput(17-35)validateHostInput(42-63)validatePortInput(70-86)
tests/Unit/Repositories/ServerRepositoryTest.php (4)
app/Repositories/ServerRepository.php (5)
findByHost(86-97)findByName(70-81)ServerRepository(15-189)loadInventory(31-43)create(48-65)tests/TestHelpers.php (1)
mockInventoryService(143-159)app/Services/InventoryService.php (1)
loadInventoryFile(97-111)app/DTOs/ServerDTO.php (1)
ServerDTO(7-17)
app/Traits/ServerValidationTrait.php (1)
app/Repositories/ServerRepository.php (2)
findByName(70-81)findByHost(86-97)
tests/Integration/Console/Server/ServerAddCommandTest.php (2)
app/Console/Server/ServerAddCommand.php (1)
execute(50-217)tests/TestHelpers.php (1)
mockSSHServiceWithBehavior(213-216)
tests/Unit/Traits/ConsoleInputTraitTest.php (3)
tests/TestHelpers.php (2)
mockCommandContainer(369-408)mockPrompter(243-254)tests/Fixtures/TestConsoleCommand.php (3)
TestConsoleCommand(27-286)setTestMethod(62-66)execute(77-110)tests/Fixtures/MockPrompter.php (1)
text(41-54)
app/Traits/ServerHelpersTrait.php (2)
app/DTOs/ServerDTO.php (1)
ServerDTO(7-17)app/Traits/ConsoleOutputTrait.php (1)
writeln(23-29)
tests/Fixtures/TestConsoleCommand.php (3)
app/Traits/ConsoleInputTrait.php (2)
getValidatedOptionOrPrompt(126-148)promptText(166-182)tests/Fixtures/MockPrompter.php (1)
text(41-54)app/Services/PrompterService.php (1)
text(33-51)
tests/Unit/Traits/ServerValidationTraitTest.php (2)
app/Traits/ServerValidationTrait.php (3)
validateNameInput(17-35)validateHostInput(42-63)validatePortInput(70-86)tests/TestHelpers.php (1)
mockServerRepository(298-311)
🪛 PHPMD (2.15.0)
tests/Fixtures/TestConsoleCommand.php
204-204: Avoid unused parameters such as '$value'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (18)
tests/Fixtures/TestConsoleCommand.php (1)
94-96: New match arms wired correctlyCovers both valid/invalid validated-prompt flows. Good dispatch coverage.
app/Repositories/ServerRepository.php (1)
83-98: findByHost mirrors findByName; clear and correctStraightforward lookup and DTO hydration. No issues.
app/Traits/ServerHelpersTrait.php (2)
22-26: Typed return on selectServerExplicit array return matches documented shape. Good.
76-90: displayServerDeets output remains consistentPresentation is concise; no behavior change. LGTM.
app/Traits/ServerValidationTrait.php (1)
13-35: Name validation refactor is cleanType check, emptiness, and uniqueness with string|null return. Good.
tests/Unit/Repositories/ServerRepositoryTest.php (2)
51-60: findByHost tests exercise both hit and missGood coverage for existing and non-existent hosts.
Also applies to: 63-63
95-107: Duplicate-host creation test validates new guardAsserts exception type and message. Solid.
tests/Unit/Traits/ConsoleInputTraitTest.php (3)
148-158: Good coverage for valid CLI option pathAsserts happy-path behavior and output. Looks solid.
160-172: Invalid CLI option flow tested correctlyVerifies error symbol, message, and null result. Matches ConsoleOutputTrait::error format.
174-186: Empty CLI value validation path coveredCorrectly expects validation error and null result when option is explicitly empty.
.cursor/rules/03-commands.mdc (1)
66-66: Method list update is accurateIncluding getValidatedOptionOrPrompt here is correct and helpful.
app/Console/Server/ServerAddCommand.php (3)
61-71: Validated name input flow is correctUses getValidatedOptionOrPrompt with required prompt and early failure. Good.
77-87: Validated host input flow is correctProper prompt + validator wiring and early failure on null.
93-107: Validated port handling is correctValidates string input, then casts to int. Matches ServerValidationTrait expectations.
tests/Integration/Console/Server/ServerAddCommandTest.php (3)
150-168: Invalid host scenario assertions match new flowExit code FAILURE and presence of error marker/message are asserted correctly.
239-239: Updated duplicate-name assertion is appropriateAligns with validator message (“already exists”).
243-280: New duplicate host test is valuableCovers repository uniqueness via findByHost and validator messaging. Good end-to-end coverage.
tests/Unit/Traits/ServerValidationTraitTest.php (1)
17-17: Add explicit type declaration to the property.The
$serversproperty lacks a type declaration, which violates PSR-12 and the coding guideline requiring explicit types. Based on the usage in tests and the return type ofmockServerRepository, this should beServerRepository.Apply this diff:
- public $servers; + public ServerRepository $servers;As per coding guidelines.
Likely an incorrect or invalid review comment.
Summary by CodeRabbit
New Features
Refactor
Tests