Clean up commands

Question

Clean up commands

CrispyDrone opened this issue 4 years ago · 1 comments

🏗 Enhancement Proposal

Commands are an instruction to nukeeper. They should only concern themselves with the information that's required to invoke a useful action from nukeeper.

I propose:

Remove all inheritance relationships between commands (will potentially involve duplicating properties across multiple commands, see natemcmaster/CommandLineUtils#432)
Extract all functionality such as validation, populating settings, initializing logging,... outside of the commands.
Every single command can have a unique command handler and command validator.

Pitch

Right now it's really difficult to get a clear overview of which options are part of a particular command since you have to go through the inheritance hierarchy and look at all the properties.

For the same reason, it can also be difficult to see where exactly a new option belongs and the impact it has on all the derived commands.

A lot of different functionality has been added to these commands making it difficult to understand what is happening where, and to test these things in isolation.

Refactoring towards command -> command validator -> command handler will promote loose coupling and a consistent style. Currently you have to look at all the commands to see where the Execute method is actually overridden—sometimes in the the terminal class such as InspectCommand, but sometimes in a non-terminal class like CollaborationPlatformCommand. The same thing is true about the validation; you have to go through all the classes of the inheritance hierarchy to get a full picture of the validation, and there's no clear indication what one of these classes will be validating on.

Setting up this small pipeline will also make it trivial to extend it such as adding logging, performance measuring, exception handling,... For this you could use an existing librray such as MediatR or write your own ICommand and IRequest interfaces and let the IoC handle the decoration.

Platforms affected (mark all that apply)

Answer 1 · 2021-04-24T01:32:33.000Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.