Looking for some general feedback regarding my server design esp. regarding the use of a generic method
This questions is a follow-up to my previous post here. Martinho has asked me to provide more information regarding my system. He has suggestion that there might be a better way to achieve what I am attempting. So, if there is a question here, I guess I am wondering if this is poor design? If so, what can be improved and how (I learn best from illustrations). Thanks.
I am designing middleware for an iPhone application at work.
Rather than explicitly calling various objects from the client, the developers want to use generics where a "Group" returns a JSON string based on a passed-in parameter. The parameter represents the first screen a user sees when he logs in. We are calling the login screen the "Dashboard".
So, when the client calls the server method:
Contracts.GroupDto IDashboardService.GetGroupById(string groupId)
{
var obj = GroupRepository.GetGroupById(groupId);
return new Contracts.GroupDto
{
...
};
}
The server uses the GroupRepository method GetGroupById to return a generic object type:
public static IList<G> GetGroupById<G>(int groupId)
{
DashboardGroupType type = (DashboardGroupType)groupId;
IList<G> result = new List<G>();
var obj = default(G);
switch (type)
{
case DashboardGroupType.Countries:
break;
case DashboardGroupType.Customers:
// this returns a list of typ IEnumerable<Customer>
obj = (G) CustomerRepository.GetAllCustomers();
break;
case DashboardGroupType.Facilities:
// this returns a list of typ IEnumerable<Facili开发者_StackOverflowty>
obj = (G) FacilityRepository.GetAllFacilities();
break;
case DashboardGroupType.Heiarchy:
break;
case DashboardGroupType.Lines:
break;
case DashboardGroupType.Regions:
// this returns a list of typ IEnumerable<string>
obj = (G) CustomerRepository.GetRegionsHavingCustomers();
break;
case DashboardGroupType.States:
// // this returns a list of typ IEnumerable<Customer>
obj = (G) CustomerRepository.GetStatesHavingCustomers();
break;
case DashboardGroupType.Tanks:
break;
default:
break;
}
result.Add(obj);
return result;
}
The object type returned is based on the parameter passed in to GetGroupById. For example, if the value is 1, the method looks at the DashboardGroupType enum:
and passes the parameter of 1, the server looks at the following enum:
public enum DashboardGroupType
{
Countries = 0,
Regions = 1,
Customers = 2,
Facilities = 3,
Lines = 4,
Tanks = 5,
States = 6,
Heiarchy = 7
}
and returns a list of regions of type IEnumerable to the calling client.
Any thoughts regarding this design (especially regarding the IList GetGroupById(int groupId) method? If you have suggestions, I would appreciate an illustration of your improvement.
Thanks in advance.
That code doesn't make a lot of sense to me. Your generic returns an IList<G>
, but the list only ever has a single item added to it. And sometimes that item is null
(or default<G>
, which will be null
when G
is a reference type).
I can't imagine why the developers would favor a GetGroupById
generic method when doing so is more complicated than having separate methods. That is, why not have GetFacilities
, GetCustomers
, etc?
Perhaps they prefer having a single service method that returns JSON to them. If there's some compelling reason to do that (although I can't imagine one, other than developer laziness), then I would suggest that you do the switch
in that method and not mess with the generic. That is:
Contracts.GroupDto IDashboardService.GetGroupById(string groupId)
{
switch ((DashboardGroupType)groupId)
{
case DashboardGroupType.Regions:
// Get the Regions list, convert to JSON, and return.
break;
// do the same kind of thing for the other group types.
}
}
Adding the generic method in this case is just needless complexity--it does nothing to simplify or increase understanding of the code. To the contrary, it makes the code harder to understand.
In general, if you have code in a generic method that acts differently based on the type parameter, then it's a very good indication that your generic method isn't really generic and probably should be implemented some other way.
Do not use switch statement. It may introduce bugs and hard to maintain. I suggest you to create a base class (abstract) with default behavior for GetGroupById() and derive classes for each of the groups DashboardGroupType. Enum are appropriate when you just need to store as a variable/property and don't have to do perform any logic based on the value. In your derived classes you can override the default behavior or keep it to default (return null). You may think of using null object design pattern as well.
You will appreciate the design when some change comes up and change is the only constant :)
You can refer to Martin Fowler's book on" Refactoring"
精彩评论