I currently have service classes that look something like this
public class UserService : IUserService
{
private IAssignmentService _assignmentService;
private ILocationService _locationService;
private IUserDal _userDal;
private IValidationDictionary _validationDictionary;
public UserService(IAssignmentService assignmentService, ILocationService locationService, IValidationDictionary validationDictionary)
{
this._assignmentService = assignmentService;
this._locationService = locationService;
this._userDAL = new UserDal();
this._validationDictionary = validationDictionary;
}
.....
private void ValidateUser(IUser user)
{
if (_locationService.GetBy(user.Location.Id) == null)
_validationDictionary.AddError("....");
if (_assignmentService.GetBy(user.Assignment.Id) == null)
_validationDictionary.AddError("....");
.....
}
}
And DAL classes that looks like this
public class UserDal: IUserDal
{
private IAssignmentDal _assignmentDal;
private ILocationDAL _locationDAL
public UserDal()
{
this_assignmentDal = new AssignmentDal();
this._locationDal = new LocationDal();
}
public int AddUser(IUser user)
{
// db call and insert user
_locationDal.Add(user.Location);
_assignmentDal.Add(user.Assignment);
}
public IUser GetUser(int id)
{
..DB Call
IUser user = new User() { userData, GetLocation(dr["Location_Id"]),GetAssignment([dr["Assignment_Id"]);
return user
}
private ILocation GetLocation(int id)
{
return new LocationDal().GetById(id);
}
private IAssignment GetAssignment(int id)
{
return new AssignmentDal().GetById(id);
}
}
I was wondering if it was considered bad design to have the Service layer talk to other Service Layer Objects and Dal talks to other Dal Objects?
Thanks in advance
Given the design of your examples, you are going to run into what I like to call dependency hell. It is certainly an option to go down the route you are taking, but it will lead to a highly coupled architecture that will likely be very difficult to maintain and refactor. If you abstract a little bit more, however, you can simplify your architecture, organize responsibilities a bit more, and separate concerns in such a way that managing your dependencies will be a lot easier.
The UserService, AssignmentService, and LocationService seem like CRUD style services. A more appropriate term for them would be Entity Services. An entity service should be solely responsible for CRUD operations of the immediate entity, and nothing else. Operations that involve multiple entities, entity relationships, etc. can be pushed to a higher-level service that can orchestrate larger-scale operations. These are often called Orchestration or Task Services.
I would recommend an approach like the following. The goals here are to simplify each service to give it the smallest responsibility scope, and control dependencies. Simplify your service contracts to reduce the responsibilities of your existing entity services, and add two new services:
// User Management Orchestration Service
interface IUserManagementService
{
User CreateUser();
}
// User Entity Service
interface IUserService
{
User GetByKey(int key);
User Insert(User user);
User Update(User user);
void Delete(User user);
}
// User Association Service
interface IUserAssociationService
{
Association FindByUser(User user);
Location FindByUser(User user);
void AssociateWithLocation(User user, Location location);
void AssociateWithAssignment(User user, Assignment assignment);
}
// Assignment Entity Service
interface IAssignmentService
{
Assignment GetByKey(int key);
// ... other CRUD operations ...
}
// Location Entity Service
interface ILocationService
{
Location GetByKey(int key);
// ... other CRUD operations ...
}
The process of creating a user and associating it with a location and assignment would belong to the UserManagementService, which would compose the lower-level entity services:
class UserManagementService: IUserManagementService
{
public UserManagementService(IUserService userService, IUserAssociationService userAssociationService, IAssignmentService assignmentService, ILocationService locationService)
{
m_userService = userService;
m_userAssociationService = userAssociationService;
m_assignmentService = assignmentService;
m_locationService = locationService;
}
IUserService m_userService;
IUserAssociationService m_userAssociationService;
IAssignmentService m_assignmentService;
ILocationService m_locationService;
User CreateUser(string name, {other user data}, assignmentID, {assignment data}, locationID, {location data})
{
User user = null;
using (TransactionScope transaction = new TransactionScope())
{
var assignment = m_assignmentService.GetByKey(assignmentID);
if (assignment == null)
{
assignment = new Assignment { // ... };
assignment = m_assignmentService.Insert(assignment);
}
var location = m_locationService.GetByKey(locationID);
if (location == null)
{
location = new Location { // ... };
location = m_locationService.Insert(location);
}
user = new User
{
Name = name,
// ...
};
user = m_userService.Insert(user);
m_userAssociationService.AssociateWithAssignment(user, assignment);
m_userAssociationService.AssociateWithLocation(user, location);
}
return user;
}
}
class UserService: IUserService
{
public UserService(IUserDal userDal)
{
m_userDal = userDal;
}
IUserDal m_userDal;
public User GetByKey(int id)
{
if (id < 1) throw new ArgumentException("The User ID is invalid.");
User user = null;
using (var reader = m_userDal.GetByID(id))
{
if (reader.Read())
{
user = new User
{
UserID = reader.GetInt32(reader.GerOrdinal("id")),
Name = reader.GetString(reader.GetOrdinal("name")),
// ...
}
}
}
return user;
}
public User Insert(User user)
{
if (user == null) throw new ArgumentNullException("user");
user.ID = m_userDal.AddUser(user);
return user;
}
public User Update(User user)
{
if (user == null) throw new ArgumentNullException("user");
m_userDal.Update(user);
return user;
}
public void Delete(User user)
{
if (user == null) throw new ArgumentNullException("user");
m_userDal.Delete(user);
}
}
class UserAssociationService: IUserAssociationService
{
public UserAssociationService(IUserDal userDal, IAssignmentDal assignmentDal, ILocationDal locationDal)
{
m_userDal = userDal;
m_assignmentDal = assignmentDal;
m_locationDal = locationDal;
}
IUserDal m_userDal;
IAssignmentDal m_assignmentDal;
ILocationDal m_locationDal;
public Association FindByUser(User user)
{
if (user == null) throw new ArgumentNullException("user");
if (user.ID < 1) throw new ArgumentException("The user ID is invalid.");
Assignment assignment = null;
using (var reader = m_assignmentDal.GetByUserID(user.ID))
{
if (reader.Read())
{
assignment = new Assignment
{
ID = reader.GetInt32(reader.GetOrdinal("AssignmentID")),
// ...
};
return assignment;
}
}
}
}
class UserDal: IUserDal
{
public UserDal(DbConnection connection)
{
m_connection = connection;
}
DbConnection m_connection;
public User GetByKey(int id)
{
using (DbCommand command = connection.CreateCommand())
{
command.CommandText = "SELECT * FROM Users WHERE UserID = @UserID";
var param = command.Parameters.Add("@UserID", DbType.Int32);
param.Value = id;
var reader = command.ExecuteReader(CommandBehavior.SingleResult|CommandBehavior.SingleRow|CommandBehavior.CloseConnection);
return reader;
}
}
// ...
}
class AssignmentDal: IAssignmentDal
{
public AssignmentDal(DbConnection connection)
{
m_connection = connection;
}
DbConnection m_connection;
Assignment GetByUserID(int userID)
{
using (DbCommand command = connection.CreateCommand())
{
command.CommandText = "SELECT a.* FROM Assignments a JOIN Users u ON a.AssignmentID = u.AssignmentID WHERE u.UserID = @UserID";
var param = command.Parameters.Add("@UserID", DbType.Int32);
param.Value = id;
var reader = command.ExecuteReader(CommandBehavior.SingleResult|CommandBehavior.SingleRow|CommandBehavior.CloseConnection);
return reader;
}
}
// ...
}
// Implement other CRUD services similarly
The conceptual layers and flow of data/objects that result from this architecture would be as follows:
Task: UserManagementSvc
^
|
-------------------------------------------------
| | | |
Entity: UserSvc UserAssociationsSvc AssignmentSvc LocationSvc
^ ^ ^ ^
| | | |
--------- - -
| | |
Utility: UserDal AssignmentDal LocationDal
^ ^ ^
| | |
---------------------------------------------
|
DB: (SQL Database)
A couple key things to note here regarding composition and dependencies. By adding the UserManagementService and composing the entity services within it, you achieve the following:
If you are careful in how you design and write each level of services, you can provide many layers of differing responsibility, complexity, and composability. An application becomes less about writing business rules, and more about composing parts to create business behavior. If a part needs to be written, it can usually be written by composing other parts, and possibly adding a small amount of additional behavior. Construction of an application becomes a lot simpler, and it is much easier to build fully functional, self-contained, reusable parts that are easier to test in isolation, and easier to deploy.
You will also have achieved Service-Orientation in the truest sense (according to Thomas Erl anyway), along with all of its benefits. ;)