Separation of model from business logic
rcdailey opened this issue · 7 comments
Several command line parsers, including this one, tightly couple the execution logic of the command (ExecuteAsync()) with the model (the properties of the class with that method).
In my case, I'd like to separate the two. I want my command classes to be "dumb data" (but still have the attributes to define the command line itself). To me, this makes things more testable as I can create dummy command objects to pass around. It also helps make it easier to share the model across different objects that implement the processing logic for that command.
I'm willing to be convinced otherwise, as this approach has a few issues, such as needing to have extra logic in my entry point method to find out which subcommand the user executed (your library does this intrinsically).
Basically how I structure my application right now (logic in my Main() method) is roughly:
- Parse command line arguments.
- Set up my composition root. This is where I instantiate my Autofac container and register all the types my application will use.
- Instantiate my logger (Serilog) which requires my DI container to resolve it.
This setup has some quirks, like how my logger depends on command line parsing because I have a command line switch (--debug) that affects the log severity printed to the console. And Serilog requires this setup to happen when the logger implementation is instantiated.
I did try having my execution logic inside the model initially, but I found it weird to perform composition root setup outside of Main(). In fact, it was happening polymorphically which just felt wrong to me.
How do others approach this issue? Should I be terribly concerned with the model and logic being one object?
If something else needs the command line arguments in a different class, they now have pulled in additional interface that they do not need, because it's not just a class of properties but methods too. keeping them tightly coupled seems to encourage monolithic classes.
Thanks in advance for any advice and opinion sharing.
I forgot to mention this, even though I sort of hinted at it:
Another reason I need to separate them: Because my composition root requires command line processing first (to influence registration behavior), I cannot register services in my subcommand classes. This would create circular dependencies. So I need: Dumb data first, register my services, then create the business logic objects that can pull in services using the now-available DI container.
Here's my entrypoint code using the CommandLineParser library, so you can see sort of what I'm dealing with.
namespace Trash
{
internal static class Program
{
private static ILogger? _logger;
private static ILogger Logger
{
get => _logger ?? throw new ApplicationException("Attempt to use the logger before it is available");
set => _logger = value;
}
private static BaseCommand ProcessCommandLine(string[] args)
{
var result = Parser.Default.ParseArguments<SonarrCommand>(args);
BaseCommand? command = null;
result
.WithParsed(parsed => command = parsed)
.WithNotParsed(errors =>
{
foreach (var error in errors)
{
Console.WriteLine($"Error: {error}");
}
throw new ArgumentException("Invalid Arguments");
});
return command!;
}
private static int Main(string[] args)
{
try
{
var command = ProcessCommandLine(args);
var container = CompositionRoot.Setup(command);
Logger = container.Resolve<ILogger>();
var config = container.Resolve<IConfigurationLoader>();
config.Load(command.Config);
var mediator = container.Resolve<IMediator>();
return (int) mediator.Send(command).Result;
}
catch (Exception e)
{
Logger.Error(e, "Application Exception");
return (int) ExitCode.Failure;
}
finally
{
Log.CloseAndFlush();
}
}
}
}The base command object:
public abstract class BaseCommand : IRequest<ExitCode>
{
[Option(HelpText =
"The base URL for your Sonarr/Radarr instance, for example `http://localhost:8989`. Required if not doing --preview.")]
public string? BaseUrl { get; set; }
[Option(HelpText = "Your API key. Required if not doing --preview.")]
public string? ApiKey { get; set; }
[Option(HelpText = "Only display the processed markdown results and nothing else.")]
public bool Preview { get; } = false;
[Option(HelpText = "Display additional logs useful for development/debug purposes.")]
public bool Debug { get; } = false;
[Option(HelpText =
"The configuration YAML file to use. If not specified, the script will look for `trash.yml` in the same directory as the `trash.py` script.")]
public string? Config { get; set; }
}And a concrete command (represents a single subcommand):
[Verb("sonarr", HelpText = "Perform operations on a Sonarr instance")]
public class SonarrCommand : BaseCommand
{
[Option(HelpText = "One or more quality definitions to set in Sonarr from the TRaSH guides.")]
public List<string> QualityDefinitions { get; init; } = new();
[Option(HelpText = "One or more release profiles to set in Sonarr from the TRaSH guides.")]
public List<string> ReleaseProfiles { get; init; } = new();
}I believe all your current issues stem from ignoring the documentation.
I did try having my execution logic inside the model initially, but I found it weird to perform composition root setup outside of Main().
The instruction for integrating CliFx with IoC is right on the frontpage (I mean readme.md).
my logger depends on command line parsing because I have a command line switch
Serilog's LoggingLevelSwitch allow dynamic configuration. Expose it to the DI directly or wrap into some sort of configuration model.
If something else needs the command line arguments in a different class, they now have pulled in additional interface that they do not need
Put the common arguments into a configuration model.
In my case, I'd like to separate the two. I want my command classes to be "dumb data" (but still have the attributes to define the command line itself).
You can do that, just pass the command instance to the command handler of your choice. Let the command to be the DTO, nobody could stop you.
ignoring the documentation
First, I did not ignore the documentation. A readme on the front page does not constitute sufficient documentation, in my opinion. It's more of a "Getting started" doc. Please do not make false assumptions. I've done a LOT of reading up to this point.
Put the common arguments into a configuration model
Example please. This doesn't make any sense to me. Why would I have a duplicated model when there's already one provided to me by CliFx? Sounds like a hack to me.
pass the command instance to the command handler of your choice
You've stated my question as the answer, which isn't the helpful. One of the major points of my question is that you cannot easily obtain the command instance without implementing ExecuteAsync(), capturing this somewhere, and referencing that. I'd prefer a way to separate parsing from execution in Main() with a property like parser.ChosenCommand that provides the instance.
I'm going to close this because I don't think I properly framed the issues I'm having and there's a lot of misunderstanding of what problem I'm trying to solve. For example, I do understand how to integrate Autofac with CliFx, but in my mind I was thinking I didn't want this because I'm trying to keep the command subclasses strictly a model, and thus they would require no services, which means no need for DI.
But I think I might have an easier time not fighting CliFx on this. I'll try to keep common logic across all commands in my base command class, and register services outside of CliFx so I can consume services in command subclasses.
Thank you for your time responding.
One of the major points of my question is that you cannot easily obtain the command instance without implementing ExecuteAsync(), capturing this somewhere, and referencing that.
Or you can just call the command handler from the command itself.
@rcdailey
To be completely honest, I can't imagine a way where configuring the logger "on-the-fly" based on command line arguments would look nice. Your example with CommandLineParser is probably as close as it gets. Also, in my personal opinion, registering services based on command line arguments is a bit weird (at least in a command line application, where the arguments are the primary input).
Combining input and execution in command was a conscious design decision because CliFx is intended to take over the entire application lifetime, which requires "owning" the command. You say "several command line parsers" do this, but I found that most actually do not (for example the above mentioned CommandLineParser). Those libraries require you to manually manage the steps between parsing of the arguments and command execution, which I wanted to avoid.
Overall, if the goal is to have dynamic verbosity configuration in the logger depending on the input, I would change the approach. Instead of ILogger, I'd inject ILoggerFactory that exposes a Create(verbosity, ...) method which constructs and returns a logger. You can then use it inside of the command to create the logger based on one of the options. If you are using Microsoft.Extensions.Logging, then it's something you should be able to do quite easily: https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.logging.loggerfactory?view=dotnet-plat-ext-5.0