I have a problem that a would like that help me. I have some service class for example, I show a part of one of them:
public class CategoriesService : ICategoriesService
{
public List<Categories>? getAll()
{
return RYCContextService.getInstance().BBDD.categories?.ToList();
}
.
.
.
}
Then, I have this class:
public class TransactionsService : ITransactionsService
{
private readonly ISplitsService splitsService;
private readonly ICategoriesService categoriesService;
private readonly IAccountsService accountsService;
public TransactionsService(ISplitsService splitsService,ICategoriesService categoriesService,
IAccountsService accountsService)
{
this.splitsService = splitsService;
this.accountsService = accountsService;
this.categoriesService = categoriesService;
}
public List<Transactions>? getAll()
{
return RYCContextService.getInstance().BBDD.transactions?.ToList();
}
.
.
.
}
That uses the category class and others, I am using injected dependencies on them, and through constructor params, I pass the services for resolving those dependencies.
This class service is used in a form, for example this:
public partial class FrmTransaction : Window
{
private Transactions? transaction;
private readonly int? accountidDefault;
private readonly IAccountsService accountsService;
private readonly ICategoriesService categoriesService;
private readonly IPersonsService personsService;
private readonly ITagsService tagsService;
private readonly ISplitsService splitsService;
private readonly ITransactionsService transactionsService;
private readonly ITransactionsStatusService transactionsStatusService;
public FrmTransaction(IAccountsService accountsService, ICategoriesService categoriesService,
IPersonsService personsService, ITagsService tagsService,ITransactionsService transactionsService,
ITransactionsStatusService transactionsStatusService, ISplitsService splitsService)
{
this.accountsService = accountsService;
this.categoriesService = categoriesService;
this.personsService = personsService;
this.tagsService = tagsService;
this.transactionsService = transactionsService;
this.transactionsStatusService = transactionsStatusService;
this.splitsService = splitsService;
InitializeComponent();
}
. . . . .
. . . .
. . . .
}
This form is called from the principal form:
public partial class MainWindow : Window
{
private readonly IAccountsService accountsService;
private readonly IAccountsTypesService accountsTypesService;
private readonly ICategoriesService categoriesService;
private readonly ICategoriesTypesService categoriesTypesService;
private readonly ITransactionsService transactionsService;
private readonly ITransactionsRemindersService transactionsRemindersService;
private readonly IExpirationsRemindersService expirationsRemindersService;
private readonly ISplitsService splitsService;
private readonly ISplitsRemindersService splitsRemindersService;
private readonly IPeriodsRemindersService periodsRemindersService;
private readonly IPersonsService personsService;
private readonly ITagsService tagsService;
private readonly ITransactionsStatusService transactionsStatusService;
public MainWindow(IAccountsService accountsService, ITransactionsService transactionsService,
ISplitsService splitsService, IExpirationsRemindersService expirationsRemindersService,
ICategoriesService categoriesService,ICategoriesTypesService categoriesTypesService,
IAccountsTypesService accountsTypesService,IPersonsService personsService,
ITagsService tagsService, ITransactionsRemindersService transactionsRemindersService,
ITransactionsStatusService transactionsStatusService, ISplitsRemindersService splitsRemindersService,
IPeriodsRemindersService periodsRemindersService)
{
InitializeComponent();
this.accountsService = accountsService;
this.transactionsService = transactionsService;
this.splitsService= splitsService;
this.expirationsRemindersService = expirationsRemindersService;
this.categoriesService = categoriesService;
this.categoriesTypesService = categoriesTypesService;
this.accountsTypesService = accountsTypesService;
this.personsService = personsService;
this.tagsService = tagsService;
this.transactionsRemindersService = transactionsRemindersService;
this.transactionsStatusService = transactionsStatusService;
this.periodsRemindersService = periodsRemindersService;
this.splitsRemindersService = splitsRemindersService;
}
private void gvTransactions_MouseDoubleClick(object sender, System.Windows.Input.MouseButtonEventArgs e)
{
if (gvTransactions.CurrentItem != null)
{
FrmTransaction frm = new FrmTransaction((Transactions)gvTransactions.CurrentItem,accountsService,categoriesService,
personsService,tagsService,transactionsService,transactionsStatusService,splitsService);
frm.ShowDialog();
loadAccounts();
loadTransactions();
refreshBalance();
}
}
....
....
.....
}
I think that there is some thing that is not correct because the code for me is very ugly, for each form that I call from the principal class increment class variables and constructor params, that it don't use on the principal form, only for passing on to the next form.
CodePudding user response：
Your code has some design issues that lead to your current situation of smelling code that is difficult to maintain. Depending on how clean you like code to be and on how big the application is (and on how much extensibility it needs) you could classify the design issues as serious:
Problem
Your constructor probably has too many parameters. This is an indicator that your class probably has too many dependencies, which means it has too many responsibilities. You should revisit your class design. This problem will make your code hard to understand, even for yourself.
Solution
Apply the Single Responsibility Principle (the 'S' in SOLID) and split up your big class into many smaller classes. Move the functionality out of the class that defines too many dependencies and has too many responsibilities. This will also help to understand your code better.
Problem
You spread RYCContextService.getInstance all over the place. It seems like you have implemented the Singleton pattern here. You should always avoid it. Especially in an environment that uses dependency injection (or any form of IoC) you don't need the Singleton.
You typically use dependency injection to make your application extensible. Adding a tight coupling to a static instance defeats the goal. It also introduces several serious issues, like degrading testability of the depending classes and therefore of your application.
You also create a potential memory leak (all references and resources of this static reference will be alive for the entire application lifetime. Reason: a static reference is treated as object root for of a reference (aka object) tree. The garbage collector can't collect a static object tree).
Solution
Instead use the lifetime management of your IoC container. For example, the globally shared instance should be injected like every other dependency. But configure the IoC container to always return the same instance (shared instance). For most IoC containers this lifetime is called Singleton like the pattern.
Problem
You create instances explicitly. This explicit creation requires the creator to know all the dependencies of the created type.
In an dependency injection context you don't want to create instances explicitly. Your code highlights the downsides:
a) introduction of tight coupling (new MyType())
b) the instantiating class declares dependencies that are only needed to internally construct other types. Those created types are not visible to the outside of the creating class (dependency hiding)
c) loss of extensibility: changing the explicitly constructed type or modifying its dependencies will require to modify the creating type.
Example:
class ClassA
{
public ClassA(IClassC classCInstance)
{
this.ClassBDependency = classCInstance;
}
private void OnButtonClick(object sender, EventArgs e)
{
string userName = "Some User";
IClassB dynamicInstance = new ClassB(userName, this.ClassBDependency);
}
}
Solution
The solution is to make the dependency visible by defining it as a class dependency. In other words, let the IoC container instantiate the type for you.
If the dependency needs to be instantiated dynamically, you would have to use the Abstract Factory pattern:
Version A that uses a Func<T> delegate as factory. Func<T> also allows to pass dynamic parameters, like user input data:
class ClassA
{
private Func<IClassB> ClassBFactory { get; }
public ClassA (Func<string, IClassB> classBFactory)
{
this.ClassBFactory = classBFactory;
}
private void OnButtonClick(object sender, EventArgs e)
{
string userName = "Some User";
IClassB dynamicInstance = this.ClassBFactory.Invoke(userName);
}
}
Version B implements the Abstract Factory pattern:
interface IClassBFactory
{
IClassB Create(string userName);
}
// The concrete type that the IoC container will inject later
class ClassBFactory : IClassBFactory
{
private IClassC ClassBDependency { get; }
// The factory alone knows the dependencies of the product.
// A factory can also depend on other abstract factories.
public ClassBFactory(IClassC classCInstance)
{
this.ClassBDependency = classCInstance;
}
public IClassB Create(string userName)
=> new ClassB(this.ClassBDependency);
}
// The type that depends on ClassB and needs to create instances of it dynamically
class A
{
private IClassBFactory ClassBFactory { get; }
public ClassA(IClassBFactory classBFactory)
{
this.ClassBFactory = classBFactory;
}
private void OnButtonClick(object sender, EventArgs e)
{
string userName = "Some User";
IClassB dynamicInstance = this.ClassBFactory.Create(userName);
}
}