views:

225

answers:

11

Trying to avoid the SomethingManager trap here...

Let's say I'm going to write a User editor which will allow administrators to create users in the system. Pretty basic functionality - view a list of existing users, create a new user, update an existing user, delete a user.

Let us also say that I decide to write a "business" class to handle these basic CRUD operations. This is probably what the interface would look like:

public interface ISomeUsefulName
{
    IList<User> FetchUsers();
    User FetchUser(int userId);
    bool SaveUser(User user);
    bool DeleteUser(int userId);
}

Inside the SaveUser() method, for example, I would validate the data (using a different class) and then actually save the data to the database (again using another class).

My question is, what should I name this class? Is this class doing too much and therefore I should split it into multiple classes?

+2  A: 

My preference would be IUserStorage or IUserStore

JaredPar
+1 for IUserStore
a paid nerd
+1  A: 

Why not just IUserCRUD? CRUD has, as oposed to 'manage' no 10 meanings.

Peter
...and of course "crud" has no other unrealted meanings. ;-)
T.E.D.
+1  A: 

How about calling it "Users" (or "AuthorizedUsers" or "CollectionOfUsers")?

ChrisW
That's my vote.
T.E.D.
+3  A: 

IUserRepository -- as in the Repository pattern.

tvanfosson
http://blogs.hibernatingrhinos.com/nhibernate/archive/2008/10/08/the-repository-pattern.aspxanother link to help clear it up.
scottm
+4  A: 

I'm seconding ChrisW's call to just name it "User".

Any time you find yourself putting the same string in the name of nearly every method, it should be removed from the method names and put in the class name.

T.E.D.
+1 to then removing the noun (i.e. the name of the class) from the name of the method.
ChrisW
He's already got a User class; unless you're suggesting that he migrate this functionality to the User class (not necessarily a bad idea), wouldn't this be a little... "collision-y"? :-)
McWafflestix
@McWafflestix: Then it could be "UserManagement" or "UserIO". Just get that name off of the methods and onto the class where it belongs.
T.E.D.
+2  A: 

IUserRepository or IUserServices.

Jamie Ide
+2  A: 

The fact that you are having trouble naming this should be a giant red flag that it is wrong.

Single Responsibility Principle (and Interface Segregation Principle) applies here. Break it up into the various operations you need.

public interface IUserList
{
    IList<User> FetchUsers();
}

public interface IUser
{
   User FetchUser(int userId);
}

public interface IUserStore
{
    bool SaveUser(User user);
    bool DeleteUser(int userId);
}

and then it gets much simpler to name them because now only one name really applies. Trust me, if you are a designer your devs will love you for making things simple to understand and use.

Robert Kozak
I think IUser is a bad choice - I would exspect the class User to implement IUser. The same with IUserList. But IUserStore is not bad, but it should contain all four methods.
Daniel Brückner
A: 

How about IUserCollection?

johnofcross
+1  A: 

It could become a generic interface.

ICrud<T> { }

Or inspired by IUserStore.

IStore<T> { }
Daniel Brückner
+2  A: 

Naming is difficult if is not respected SRP :) But members naming is often misused.

In your case I'll do something like this:

  • the responsibility of implementation is to cover specified contract of persistence
  • "who" is under fire

Thinks without voice - persistence is done for user and a relevant name can be IUserRepository - methods are not more than for CRUD - because of the fact that the IUserRepository is for user, is not necessary to have UserSave, UserUpdate because it brakes the generic usage manner

The Magic Is Here ... just do this:

public interface IRepository<TYPE, KEY>{
  IList<TYPE> GetAll(KEY key);
  TYPE GetById(KEY key);
  void Save(TYPE obj);
  void Update(TYPE obj);
  void Delete(Key key);
}

Is it difficult ? What to do with a custom one?

public interface IUserRepository : IRepository<User, int>
{
   IList<User> GetAllMyFavorites(ICriteria crit);
   IList<Events> GetHistoryByUser(User user);   
}

In the code using an IoC container you can do easily

public UserController {
  private _userRepository = null;
  private _eventsRepository = null;

  public UserController(IUserRepository userRepository, 
  IRepository<Events,int> eventsRepository) 
  // if you are doing here just CRUD use the generic signature
  {
    _userRepository = userRepository;
    _eventsRepository = eventsRepository;
  }

  public MarkItAsGoldPartener(int userId){
     var user = userRepository.GetById(userId);
     user.PartnerType = PartnerTypes.Gold;
     userRepository.Save(user); // the user in member name is useless
     eventsRepository.Save(new Event(){Message = "The user" + UserId + "is golden" });
  }
}

good luck :)

ruslander
+1 Good answer. You thought it out better than I did.
Robert Kozak
A: 

I'd go with UserActions. This describes the set of functionality you want to do; it avoids the trap of calling it a collection (since it doesn't actually collect anything, simply retrieves a collection).

But I'd also rethink having this class in this form in the first place. It looks like what you're trying to put in place is a persistence manager; are there any other types of objects that you're going to want to persist in this manner? Can you extract any common functionality that can then be derived to a base class? Perhaps a "PersistenceManager" class or somesuch? Then, if it's absolutely necessary (and I'm not certain it would be), you could derive a "UserPersistenceManager" that would operate on User objects alone. (I believe it might not be necessary because you may be able to perform everything you need just from the PersistenceManager; only your specific implementation can tell you that, though.)

McWafflestix