views:

75

answers:

3

Firstly, This might seem like a long question. I don't think it is... The code is just an overview of what I'm currently doing. It doesn't feel right, so I am looking for constructive criticism and warnings for pitfalls and suggestions of what I can do.

I have a database with business objects.
I need to access properties of parent objects.
I need to maintain some sort of state through business objects.

If you look at the classes, I don't think that the access modifiers are right. I don't think its structured very well. Most of the relationships are modelled with public properties. SubAccount.Account.User.ID <-- all of those are public..

Is there a better way to model a relationship between classes than this so it's not so "public"?

The other part of this question is about resources:

If I was to make a User.GetUserList() function that returns a List, and I had 9000 users, when I call the GetUsers method, it will make 9000 User objects and inside that it will make 9000 new AccountCollection objects. What can I do to make this project not so resource hungry?

Please find the code below and rip it to shreds.

public class User {

   public string ID {get;set;}
   public string FirstName {get; set;}
   public string LastName {get; set;}
   public string PhoneNo {get; set;}

  public AccountCollection accounts {get; set;}

  public User {
     accounts = new AccountCollection(this);
  }

  public static List<Users> GetUsers() {
     return Data.GetUsers();
  }

}

public AccountCollection : IEnumerable<Account> {
  private User user;

  public AccountCollection(User user) {
     this.user = user;
  }

  public IEnumerable<Account> GetEnumerator() {
     return Data.GetAccounts(user);
  }
}


public class Account {

   public User User {get; set;}  //This is public so that the subaccount can access its Account's User's ID
   public int ID;
   public string Name;

   public Account(User user) {
      this.user = user;
   }

}

public SubAccountCollection : IEnumerable<SubAccount> {
  public Account account {get; set;}

  public SubAccountCollection(Account account) {
     this.account = account;
  }

  public IEnumerable<SubAccount> GetEnumerator() {
     return Data.GetSubAccounts(account);
  }
}


public class SubAccount {
   public Account account {get; set;}    //this is public so that my Data class can access the account, to get the account's user's ID.

   public SubAccount(Account account) {
      this.account = account;  
   }

   public Report GenerateReport() {
       Data.GetReport(this);
   }

}


public static class Data {

  public static List<Account> GetSubAccounts(Account account) {

      using (var dc = new databaseDataContext()) {
          List<SubAccount> query = (from a in dc.Accounts
                                where a.UserID == account.User.ID  //this is getting the account's user's ID
                                select new SubAccount(account) {
                                    ID = a.ID,
                                    Name = a.Name,
                                }).ToList();
      }

  }

  public static List<Account> GetAccounts(User user) {

     using (var dc = new databaseDataContext()) {
         List<Account> query = (from a in dc.Accounts
                               where a.UserID == User.ID  //this is getting the user's ID
                               select new Account(user) {
                                   ID = a.ID,
                                   Name = a.Name,
                               }).ToList();
     }
  }

  public static Report GetReport(SubAccount subAccount) {

     Report report = new Report();
     //database access code here
     //need to get the user id of the subaccount's account for data querying.
     //i've got the subaccount, but how should i get the user id.
     //i would imagine something like this:
     int accountID = subAccount.Account.User.ID;
     //but this would require the subaccount's Account property to be public.
     //i do not want this to be accessible from my other project (UI).
     //reading up on internal seems to do the trick, but within my code it still feels
     //public. I could restrict the property to read, and only private set.

     return report;
  }

  public static List<User> GetUsers() {

     using (var dc = new databaseDataContext()) {
         var query = (from u in dc.Users
                     select new User {
                       ID = u.ID,
                       FirstName = u.FirstName,
                       LastName = u.LastName,
                       PhoneNo = u.PhoneNo
                     }).ToList();

         return query;
     }
  }

}
+2  A: 

Lazy loading - do not make the AccountCollection objects inside the Users unless it is accessed. Alternatively, you can have it retrieve the account collections at the same time as the users and avoid 1 + 9000 database trips.

Have more selective collection retrieval methods (i.e. what are you going to do with 9000 users? If you need all their information, it's not so much of a waste).

Have smaller "digest" objects which are used for long lists like dropdowns and lookups - these objects are simple read-only business objects - usually containing only a small amout of information and which can be used to retrieve fully blown objects.

If you are doing pagination, is the list of users loaded once and stored in the web server and then paginated from the cached copy, or is the collection loaded every time from the database and pagination done by the control?

Cade Roux
How could I use lazy loading here? Could you give an example? or link me to an example?I will only be using firstname, lastname, and userid for a list of users able to be selected. (for the userslist) I'm going to use some pagination for the list on an ASP.NET client application. With ListView component with the Pagination control I believe the ListView requires the entire collection to be databound, and then the pagniation control does the rest.
Mike
List is loaded once, and then pagination happens from cached copy.Bit the cached copy is like 9000 records long.
Mike
@Mike - I edited answer somewhat - You also want to avoid those public set properties - this can allow your object model to be altered outside of the control of its objects. It's fine to be able to "dot" around your object graph, but you typically don't want things to be set (and if you do, you usually are going to write the set method yourself).
Cade Roux
So you're saying, make the AccountCollection null when i make the user, but when i access the Account collection, check to see if it is null, then make a new account collection, otherwise just use the accountcollection?
Mike
Yes, thats exactly my concern. Because they are public, they can be edited outside the scope of what I want. I'm not sure how to make them more private but accessible for my Data class.
Mike
@Mike - David Hall's answer has a good survey of a variety of techniques and design principles which all go into this seemingly simple example. You can have a public get, private set.
Cade Roux
+3  A: 

This answer has ended up containing a lot of buzz word headings. Hopefully I explain each one and why it applies here. I think each concept I introduce below is worth considering - they aren't always applicable but I find they are all things I personally find valuable when I think about the structure of a system.

Single Responsibility

Start by thinking about the responsibility of each object - what is its job? Generally you'll find a better design once you decide on a single job for each class. Currently a lot of your classes are doing too much, holding logic that should really exist as services.

The first example of the above is your User class:

public class User { 

   public string ID {get;set;} 
   public string FirstName {get; set;} 
   public string LastName {get; set;} 
   public string PhoneNo {get; set;} 

  public AccountCollection accounts {get; set;} 

  public User { 
     accounts = new AccountCollection(this); 
  } 

  public static List<Users> GetUsers() { 
     return Data.GetUsers(); 
  } 

} 

Why does this provide a method that retrieves users from the data source? That functionality should be moved out into a users service.

Another key example of this is the GenerateReport method on the SubAccount - don't have your report generation logic so tightly tied to the SubAccount object. Splitting this out will give you more flexibility and reduce the change of changes to your SubAccount breaking the report logic.

Lazy Loading

Again looking at your User class - why does it load all the users accounts on instantiation? Are these objects always going to be used every time you work with a User?

It would generally be better to introduce lazy loading - only retrieve an account when you need it. Of course there are times when you want eager loading (if you know you will want the object soon so want ot reduce database access) but you should be able to design for these exceptions.

Dependency Injection

This sort of follows on from both the lazy loading point and the single responsibility point. You have a lot of hard coded references to things like your Data class. This is making your design more rigid - refactoring your data access to introduce lazy loading, or changing the way that user records is retrieve is much harder now that many classes are all directly accessing the data access logic.

Digest objects

Hat tip to Cade Roux - I'd never heard the term Digest objects, usually called them light weight DTOs.

As Cade says, there is no reason to retrieve a rich list containing fully functioning user objects if all you are doing is displaying a combo box of user names that is bound to the unique ids.

Introduce a light weight user object that only stores the very basic user information.

This is again another reason to introduce a services/repository abstraction, and some sort of dependency injection. Changing the types of objects retreived from the data store becomes much easier when you have encapsulated your data retrieval away from your actual objects, and when you are not tightly bound to your data access implementation.

Law of Demeter

Your objects know too much about the internal structure of each other. Allowing drill down through a User to the Accounts and then to the SubAccount is muddling the responsibility of each object. You are able to set sub account information from the user object when arguably you shouldn't be.

I always struggle with this principle, since drilling through the heirarchy seems very convenient. The problem is that it will stop you thinking proberly about the encapsulation and role of each object.

Perhaps don't expose your Account object from user - instead try introducing properties and methods that expose the relevant members of the Account object. Look at a GetAccount method instead of a Account property, so that you force yourself to work with an account object rather than treat it as a property of the User.

David Hall
The example that I've put above in my question is a MUCH smaller version of a bigger project. I guess I just want to fix it before its gets out of hand.With the GetReport on the subaccount object - this report is specifically only for the subaccount. There are no other reports generated, except the subaccount. I thought it fits there.Yeah, the GetUsers method should probably be in a UsersService.You think that I should be using Methods instead of properties to get the relations? Interesting. Do you think that I should store those relations inside the object still?
Mike
I'm still not quite sure what you mean by the Dependency Injection part of your response. How would dependency injection work here?
Mike
I mean, from what I understand about dependency injection, it initializes an object with itself through the object's constructor. Like I am currently doing. Every user has at least one account, so the AccountsCollection will be apart of every user, but probably not required until it needs to be accessed.
Mike
A: 

Okay, just a quick comment on the code as you have it at the moment.

public class SubAccount {
   public Account account {get; set;}    //this is public so that my Data class can access the account, to get the account's user's ID.

    public SubAccount(Account account) {
        this.account = account;  
    }
    [snip]
}

You don't even need a setter on the Account property if it is passed in on the ctor and then assigned to the backing field.

Setting a property by passing it on on the ctor can be a very good practice to follow, but it falls over if your data object is going to be serialized, i.e. if it is retrieved or sent to a web service - in this case you will need at least an internal setter on the Account property.

Edit: you can still use this DI type behaviour on the constructor, but the problem is that this constructor will not get called when the object is rehydrated from a serialized form (i.e. when it has been passed to/from a web service). So if you are going to use web services you will additionally need to have a parameterless constructor, and either a public or internal setter on the Account property (if you are going for an internal setter then you will also need to specify the InternalsVisibleToAttribute in your AssemblyInfo.cs file.

slugster
But isn't that how dependency injection works?I might need to use web services in the future.
Mike
Check my edit :)
slugster