A Design question [offtopic?]
-
Hi all, I posted this on the General Discussion board, but it seems a little dead in there. Hopefully this isn't too offtopic for this board..... I was hoping for some advice on 'best practices' regarding the design of an app I'm working on. It's a subscription-based web app in C#. Member data is stored in a DB table cunningly named Members. I've implemented a fairly simple Member class which essentially wraps access to a row in the table, with functions like: static public Member FetchFromDB( int iMemberID ); public void UpdateInDB(); etc. and getters/setters to map the contents of the Members row, like: public DateTime ExpiryDate{ get{ return _expiryDate; } } public StatusEnum Status{ get{ return _status; } } etc. I'm happy with this class as described so far. It does one job, maintains a constant level of abstraction/encapsulation, etc. However, My app also needs functionality like 'Check whether a member's ExpiryDate is in the past, and if so update their Status to Expired', or 'Update LastOnline to now'. The way I've been doing this so far is to add methods to Member such as UpdateLastOnlineToNow(), CheckExpiryDate(), etc. Obviously this works fine, but I'm finding myself adding more and more 'utility' methods to Member, and I'm starting to think that Member has a 'bad smell' - it's still a wrapper for the DB, but it's also getting more and more higher-level functionality mixed into it. This class is just an example, I'm also getting similar bad smells with various similar classes in my app (Message, for example, which encapsulates my message sending/receiving system). So my question is, what do all you wise and esteemed CPians think? Have I been reading my Patterns books a bit too religiously? If I not, what would be the best way to refactor this? One thought I had is to break Member out into 3 classes, a low-level wrapper to the DB, a high-level wrapper that uses the low-level wrapper internally, and a utility class that also uses the low-level wrapper. Is this overkill? Any other suggestions (or any thoughts at all) gratefully appreciated! Cheers, Pete
-
Hi all, I posted this on the General Discussion board, but it seems a little dead in there. Hopefully this isn't too offtopic for this board..... I was hoping for some advice on 'best practices' regarding the design of an app I'm working on. It's a subscription-based web app in C#. Member data is stored in a DB table cunningly named Members. I've implemented a fairly simple Member class which essentially wraps access to a row in the table, with functions like: static public Member FetchFromDB( int iMemberID ); public void UpdateInDB(); etc. and getters/setters to map the contents of the Members row, like: public DateTime ExpiryDate{ get{ return _expiryDate; } } public StatusEnum Status{ get{ return _status; } } etc. I'm happy with this class as described so far. It does one job, maintains a constant level of abstraction/encapsulation, etc. However, My app also needs functionality like 'Check whether a member's ExpiryDate is in the past, and if so update their Status to Expired', or 'Update LastOnline to now'. The way I've been doing this so far is to add methods to Member such as UpdateLastOnlineToNow(), CheckExpiryDate(), etc. Obviously this works fine, but I'm finding myself adding more and more 'utility' methods to Member, and I'm starting to think that Member has a 'bad smell' - it's still a wrapper for the DB, but it's also getting more and more higher-level functionality mixed into it. This class is just an example, I'm also getting similar bad smells with various similar classes in my app (Message, for example, which encapsulates my message sending/receiving system). So my question is, what do all you wise and esteemed CPians think? Have I been reading my Patterns books a bit too religiously? If I not, what would be the best way to refactor this? One thought I had is to break Member out into 3 classes, a low-level wrapper to the DB, a high-level wrapper that uses the low-level wrapper internally, and a utility class that also uses the low-level wrapper. Is this overkill? Any other suggestions (or any thoughts at all) gratefully appreciated! Cheers, Pete
I suppose it all depends on the coder's style. I'd do it this way: 1. I would create a Member class which would be an abstraction of a "real life" Member, or in other words, a Member object. It would have all the cunningly named strongly typed properties like string Name, int Id, date ExpirationDate, etc, and not much else. U could even make it a struct, but if it gets passed around a lot, it might be better to keep it as a class. 2. Then I would create a DBManager Class that would fetch, insert and update Member instances from/into the DB. This would be like your barebone original Member class but all methods/properties would be strongly typed to the Member class when necesarry:
public Member GetMember(int memberId) { //Implmentation } public void UpdateMember(Member changedMember) { //Implementation } etc...3. And finally I would create a utility class that would perform all the functionalities upon the Member instances such as the ones you mentioned above: "UpdateLastOnLine", etc. All changes in the member instance would then be updated into the DB through the DBManager.
public void UpdateLastOnLine(Memeber member) { //Implmentation } etc...Hope this helps....if not quite your coding style, it might still give u some ideas. It might be an overkill for the true requirements of your app, but I always try to make my stuff in a orderly and structured fashion, even if its really not necessary.
-
I suppose it all depends on the coder's style. I'd do it this way: 1. I would create a Member class which would be an abstraction of a "real life" Member, or in other words, a Member object. It would have all the cunningly named strongly typed properties like string Name, int Id, date ExpirationDate, etc, and not much else. U could even make it a struct, but if it gets passed around a lot, it might be better to keep it as a class. 2. Then I would create a DBManager Class that would fetch, insert and update Member instances from/into the DB. This would be like your barebone original Member class but all methods/properties would be strongly typed to the Member class when necesarry:
public Member GetMember(int memberId) { //Implmentation } public void UpdateMember(Member changedMember) { //Implementation } etc...3. And finally I would create a utility class that would perform all the functionalities upon the Member instances such as the ones you mentioned above: "UpdateLastOnLine", etc. All changes in the member instance would then be updated into the DB through the DBManager.
public void UpdateLastOnLine(Memeber member) { //Implmentation } etc...Hope this helps....if not quite your coding style, it might still give u some ideas. It might be an overkill for the true requirements of your app, but I always try to make my stuff in a orderly and structured fashion, even if its really not necessary.
Hi Skynyrd, Thanks for the response. To clarify, the DBManager would just be responsible for wrapping the Member table in the DB, or wrapping all the tables? I'm also not sure how the coupling would work with the three classes you mention. The way I see it, Member should be hiding/abstracting over some of the internal properties of a Member. For example, LastOnline shouldn't be settable, it should just be updatable to DateTime.Now. That being the case, both DBManager and the utility class would need access to the internals of Member, and I'm not sure how best to accomplish that. That was why I thought of having a low-level wrapper (which would expose those internals), and then a higher-level wrapper, which would be, as you say, a Member 'object'. But with that design, I'm not sure if it's feasible to have a seperate utility class. The utility class would need access to the internals, but the client of the utility class presumably wouldn't be able to access them. If this were C++, I'd consider making the utility class a friend of the lower-level wrapper, but AFAIK that isn't possible in C#. I hope you don't think I'm being argumentative about this, I'm honestly just trying to figure the best way to design this sucker. I do appreciate your input! And yes, as you say, all of this probably /is/ overkill for the app, but like you I like to try and keep my design as clean as possible (within reason!). Cheers, Pete
-
Hi Skynyrd, Thanks for the response. To clarify, the DBManager would just be responsible for wrapping the Member table in the DB, or wrapping all the tables? I'm also not sure how the coupling would work with the three classes you mention. The way I see it, Member should be hiding/abstracting over some of the internal properties of a Member. For example, LastOnline shouldn't be settable, it should just be updatable to DateTime.Now. That being the case, both DBManager and the utility class would need access to the internals of Member, and I'm not sure how best to accomplish that. That was why I thought of having a low-level wrapper (which would expose those internals), and then a higher-level wrapper, which would be, as you say, a Member 'object'. But with that design, I'm not sure if it's feasible to have a seperate utility class. The utility class would need access to the internals, but the client of the utility class presumably wouldn't be able to access them. If this were C++, I'd consider making the utility class a friend of the lower-level wrapper, but AFAIK that isn't possible in C#. I hope you don't think I'm being argumentative about this, I'm honestly just trying to figure the best way to design this sucker. I do appreciate your input! And yes, as you say, all of this probably /is/ overkill for the app, but like you I like to try and keep my design as clean as possible (within reason!). Cheers, Pete
DBManager could be used for wrapping all tables in the DB. If ur only updating and selecting, u could still keep the class public interface pretty simple and wrap all tables. It also depends on how u are encapsulating other objects equivalent to Members. U could even use a generic object and through Reflection and Attributes obtain the properties of the objects that represent fields in your underlying DataBase tables and thus create a very decoupled DB Manager with signatures like: Update(object changedRegistry) etc. (U could create a custom TableFieldAttribute(string fieldName) to mark the properties that represent real fields in the DB tables in order to acheive this...this comes in handy if u expect using additional properties in these objects that dont necessarily represent underlying fields). Concerning the threeway implementation, ur right about the LastOnline method, this is probably where overkill comes in :p. The utility class would not be really necessary if only these type of methods appear. Maybe u could keep the implementation to just two classes (DBManager and Member) and Member would take care of methods similar to LastOnline, as it is logical that only Member should have access to the LastOnline setter. On the other hand, if some utilities require extensive work with Member instances then the threeway solution would be plausible, keeping methods like LastOnline in the Member class as a tool for the Utility class.
-
DBManager could be used for wrapping all tables in the DB. If ur only updating and selecting, u could still keep the class public interface pretty simple and wrap all tables. It also depends on how u are encapsulating other objects equivalent to Members. U could even use a generic object and through Reflection and Attributes obtain the properties of the objects that represent fields in your underlying DataBase tables and thus create a very decoupled DB Manager with signatures like: Update(object changedRegistry) etc. (U could create a custom TableFieldAttribute(string fieldName) to mark the properties that represent real fields in the DB tables in order to acheive this...this comes in handy if u expect using additional properties in these objects that dont necessarily represent underlying fields). Concerning the threeway implementation, ur right about the LastOnline method, this is probably where overkill comes in :p. The utility class would not be really necessary if only these type of methods appear. Maybe u could keep the implementation to just two classes (DBManager and Member) and Member would take care of methods similar to LastOnline, as it is logical that only Member should have access to the LastOnline setter. On the other hand, if some utilities require extensive work with Member instances then the threeway solution would be plausible, keeping methods like LastOnline in the Member class as a tool for the Utility class.
I think the DBManager concept is interesting, particularly the use of Reflection and Attributes. However, I think it /would/ be overkill in this case :) Also, it could get complicated for certain things - e.g. the Member table contains pictures, which I don't want to fetch every time I get member info. Obviously, the DBManager concept could be extended to cover that kind of thing, but then we're certainly deep in overkill land. The reservations I have with the design you suggest is that I'm still left with a Member class that's got various levels of abstraction. While on the one hand it's exposing simple getter/setters, and it's also doing higher-level abstraction utility functionality - e.g. UpdateLastOnlineToNow(), CheckExpiryDate(), UpdateWithInputFromWebForm() (I should have mentioned that one earlier I guess). Any other suggestions ;) No, seriously, thanks for the input. It's been helpful. Keep it coming ;) Cheers, Ptete
-
I think the DBManager concept is interesting, particularly the use of Reflection and Attributes. However, I think it /would/ be overkill in this case :) Also, it could get complicated for certain things - e.g. the Member table contains pictures, which I don't want to fetch every time I get member info. Obviously, the DBManager concept could be extended to cover that kind of thing, but then we're certainly deep in overkill land. The reservations I have with the design you suggest is that I'm still left with a Member class that's got various levels of abstraction. While on the one hand it's exposing simple getter/setters, and it's also doing higher-level abstraction utility functionality - e.g. UpdateLastOnlineToNow(), CheckExpiryDate(), UpdateWithInputFromWebForm() (I should have mentioned that one earlier I guess). Any other suggestions ;) No, seriously, thanks for the input. It's been helpful. Keep it coming ;) Cheers, Ptete
I'm not sure that the Member class should not be able to perform some simple higher-level abstraction utilities. If the actions are solely meaningful to the member class, then there is no problem in implementing them in that class. For example, LastOnline and CheckExpiryDate might be only meaningful to the member class. Thus it would be reasonable to implement it in that class, even if its a higher level abstraction. Particularly, I would implement the CheckExpiryDate() as a boolean property HasExpired (question of tastes here :-D). On the other hand, more general functionality like UpdateWithInputFromWebForm might be a more general utility that might be meaningful to more objects than Member instances and should be implemented in a Utility class. My first design wouldnt be valid anyhow, as the Utility class should not be strongly typed to the Member class, and the update could and should be done in a generalize way (again with Reflection through a Dictionary collection: property name - new value for example u could acheive this) To make it short, what I always try to do is isolate all DB Management from the real objects I'm using in my application. In your original design, your Member class was implementing the DB update logic too, and I think thats where I would change the abstraction of the objects. Implementing the utility class depends again on how complicated your app is really going to be, but I dont think implementing high level abstractions in the Member class is a design fault if those utilities are only applicable to the Member class. I think we need some second view about this, so any more suggestions are welcome :)