patterncsharpMinor
Does the class have unnecessary and violating Single responsibility principle method?
Viewed 0 times
unnecessarytheviolatingprinciplemethodsingleresponsibilitydoesandclass
Problem
I encountered with the following code in the work. The
My questions are:
-
If it's not a factory so why
example:
Inside
-
So, is this implementation correct? Doesn't it have something wrong?
MyItemCoordinator should receive MyItem objects, process them, add them to MyItemList collection and then pass them to the listeners.My questions are:
- When I saw
NewMyItemmethod I thought thatMyItemCoordinatoris a factory but when I sawNewMyItem's implementation I found out that it creates, sorts, calling to listeners and it's notstatic. SoMyItemCoordinatoris not a factory andNewMyItemmethod does way more things that it should to do according to its name, right?
-
If it's not a factory so why
NewMyItem method receives so many params, creates an object, adds to the collection? Why not to do:example:
var coordinator = new MyItemCoordinator();
var myItem = new MyItem(...PARAMS..)
coordinator.AddMyItemToList(myItem)Inside
AddMyItemToList we can sort and call the listeners. This is more clear way to do, isn't it?-
So, is this implementation correct? Doesn't it have something wrong?
public class MyItemCoordinator
{
...
public IList MyItemList = new List();
public void NewMyItem(int id, ...15_MORE_PARAMS)
{
MyItem myItem = new MyItem(id,...ONLY_5_PARAMS_WITHIN_15_ARE_USED);
AddMyItemToList(myItem);
SortMyItemList();
if (MyItemDetectedHandler != null)
MyItemDetectedHandler(this);
}
public void AddMyItemToList(MyItem myItem)
{
//makes some checking and adds myItem to MyItemList
}
...
}Solution
You're right, but you should take this to its logical conclusion.
With this design you can reliability test each component in isolation. Merging these responsibilities into a single class makes testing difficult and blocks alternate implementations.
- Creating item belongs to a factory class.
- Tracking a list of items (coordinating?) is another responsibility.
- Handling the new item (or list? Not clear from the code) belongs to a third class.
With this design you can reliability test each component in isolation. Merging these responsibilities into a single class makes testing difficult and blocks alternate implementations.
Context
StackExchange Code Review Q#47413, answer score: 2
Revisions (0)
No revisions yet.