none
Who's Responsible Of These Actions? RRS feed

  • General discussion

  • Hey everyone,

    I am currently building an ASP.NET MVC website with the Entity Framework Code First as my ORM. I am also using the repository pattern:
    - Repository retrieves and adds/deletes from the data store (MS SQL Server in my case)
    - Service classes filter the data and deliver it to the view
    - An MVC Web Application takes care of the application logic and rendering the views

    I have a POCO User class (as well as many others) which currently contains nothing but properties that will be mapped to table columns when EF generates the database. I also have a UsersManager class which should be self explanatory.

    According to my project requirements, the User can do several things (events?) including:
    - Post a comment
    - Add a listing to his WatchList
    - Send a private message to another user
    - Bid on an auction (listing)
    - ...etc

    Question:
    Where does the logic of the events mentioned above go? Is the User class responsible of implementing this logic, or is it the UsersManager class? Or maybe another service class which makes use of other service classes (see below):

    namespace Sharwe.Services.Business
    {
      public class BiddingService
      {
        private UsersManager userManager = new UsersManager();
        private ListingsManager listingsManager = new ListingsManager();
        private IValidationDictionary validationDictionary;
    
        public BiddingService(IValidationDictionary vd)
        {
          validationDictionary = vd;
        }
    
        public bool PlaceBid(int userId, int listingId, Bid bid)
        {
          User user = userManager.GetUserById(userId);
          Listing listing = listingsManager.FindListingById(listingId);
          if (bid.Amount <= listing.Bids.Max(a => a.Amount))
            validationDictionary.AddError("InvalidBid", "You need to place a bid that is greater than this listing's highest bid.");
          
          if(listing.User.ID.Equals(userId))
            validationDictionary.AddError("InvalidBidder", "A user cannot bid on his own listings.");
          
          if (user.Credits < Constants.RepostingCredits || user.PromotionalCredits < Constants.RepostingCredits)
            validationDictionary.AddError("InsufficientCredits", "You need to have at least " + Constants.RepostingCredits 
              + " credits or promotional credits in your wallet to place a bid.");
          
          try
          {
            user.Bids.Add(bid);
            listing.Bids.Add(bid);
            userManager.Save();
            listingsManager.Save();
            return validationDictionary.IsValid;
          }
          catch (Exception ex)
          {
            throw ex;
          }
        }
      }
    }
    

    I do not like it very much though and I'm not really sure about it. I'd really appreciate some thoughts about this...

     

    • Edited by KassemD Tuesday, February 1, 2011 11:54 AM formatted the code to look more visible
    Tuesday, February 1, 2011 11:52 AM

All replies

  • Hi KassemD,

    These are just opinions and they are based only on your few lines of code, so take these comments with a pinch of salt.

    What I noticed is a bid is added to both a user and a listing, I don't like this (although I understand why).

    In my minds eye a bid is made by a user for an item, that might be wrong, depends on what the client wants to happen, but lets stick with that for now.

     

    The code I would write for that would be....

    user.bidFor(listing, bid)

    actually I'd prefer something like this....

    user.bidFor(item, amount)

    .... listing is a verb not a noun but that's just semantics. 

     

    In term of watching an item the code I would want to write would be....

    user.watch(item)

    .... for posting a comment... what is he commenting against? an item? a seller?

    ... and for sending a message to another user I'd want to write the following code.

    user.contact(otherUser, message)

    ....so couple of things one it looks like it's all going through the user object... and the other thing is it looks like there is no need for a Bid object.

    In fact is there a need for the UserManager class at all in the above example.... if I wanted to write the above I'd aim for this sort of code.

     

        public bool PlaceBid(int userId, int listingId, int amount)
        {
          User user = new User(userId);
          Listing listing = new Listing(listingId);

          bool validBid = ValidateBid(user, listing, amount)
          if (validBid)
              user.bidFor(listing, amount)
          return validBid;
        }


        // why seperate it? you can now validate the bid without placing the bid
        private bool ValidateBid(User user, Listing listing, int amount)
        {
          if (amount <= listing.HighestBid)
            validationDictionary.AddError("InvalidBid", "You need to place a bid that is greater than this listing's highest bid.");
         
          if(listing.Owner == user)
            validationDictionary.AddError("InvalidBidder", "A user cannot bid on his own listings.");
         
          if (user.Credits < Constants.RepostingCredits || user.PromotionalCredits < Constants.RepostingCredits)
            validationDictionary.AddError("InsufficientCredits", "You need to have at least " + Constants.RepostingCredits
              + " credits or promotional credits in your wallet to place a bid.");

          return validationDictionary.IsValid;
        }

     

    In fact I'd move the validation into the listing, something like this.

        public bool PlaceBid(int userId, int listingId, int amount)
        {
          User user = new User(userId);
          Listing listing = new Listing(listingId);

          if (listing.IsValidBid(amount))
              validation.AddError("InvalidBid", "You need to place a bid that is greater than this listing's highest bid.");

          if (user.Owns(listing))
              validation.AddError("InvalidBidder", "A user cannot bid on his own listings.");

          if (!user.HasCredit())
              validation.AddError("InsufficientCredits", "You need to have at least " +
                                    Constants.RepostingCredits +
                                " credits or promotional credits in your wallet to place a bid.");

          bool validBid = validation.IsValid;
          if (validBid)
              user.bidFor(listing, amount)
          return validBid;
        }





        // should be in the listings class
        public bool IsValidBid(int amount)
        {
          if (amount <= this.HighestBid)
             return false;
        }




        // belongs to the user class; the user owns the listing
        public bool Owns(Listing listing)
        {
          if(listing.Owner == this)
            return true;
        }

       
        // belongs to the user class; the user has credit
        public bool HasCredit()
        {
          // logic reversed
          if (user.Credits >= Constants.RepostingCredits || user.PromotionalCredits >= Constants.RepostingCredits)
            return true;
        }

     

     

    Anyway that might be no use to you but I do like refactoring code.  :)

     


    …we each have more potential than we might ever presume to guess. (Blog: http://dsmyth.blogspot.com/)
    Tuesday, February 1, 2011 2:14 PM
  • Sorry forgot to extract the validation out to it's own mothod.... this would be my final listing. Short and sweet methods.

     

        public bool PlaceBid(int userId, int listingId, double amount)
        {
          User user = new User(userId);
          Listing listing = new Listing(listingId);

          bool validBid = ValidateBid(user, listing, amount);
          if (validBid)
              user.bidFor(listing, amount)
          return validBid;
        }



        public bool ValidateBid(User user, Listing listing, double amount)
        {
          if (listing.IsValidBid(amount))
              validation.AddError("InvalidBid", "You need to place a bid that is greater than this listing's highest bid.");

          if (user.Owns(listing))
              validation.AddError("InvalidBidder", "A user cannot bid on his own listings.");

          if (!user.HasCredit())
              validation.AddError("InsufficientCredits", "You need to have at least " +
                                    Constants.RepostingCredits +
                                " credits or promotional credits in your wallet to place a bid.");
          return validation.IsValid
        }






        // should be in the listings class
        public bool IsValidBid(double amount)
        {
          if (amount <= this.HighestBid)
             return false;
        }





        // belongs to the user class; the user owns the listing
        public bool Owns(Listing listing)
        {
          if(listing.Owner == this)
            return true;
        }

       
        // belongs to the user class; the user has credit
        public bool HasCredit()
        {
          // logic reversed
          if (user.Credits >= Constants.RepostingCredits || user.PromotionalCredits >= Constants.RepostingCredits)
            return true;
        }


    …we each have more potential than we might ever presume to guess. (Blog: http://dsmyth.blogspot.com/)
    Tuesday, February 1, 2011 2:18 PM
  • Hi Derek,

    Thank you for the detailed explanation. This is definitely not of "no-use" to me, because this is actually what I'm looking for! :)

    A few comments though:

    public bool PlaceBid(int userId, int listingId, double amount)
        {
          User user = new User(userId);
          Listing listing = new Listing(listingId);
    
          bool validBid = ValidateBid(user, listing, amount);
          if (validBid)
              user.bidFor(listing, amount)
          return validBid;
        }
    

    Why would I create new instances of a user and a listing here? Shouldn't I retrieve those from my repository (using my service classes)?

    Also, is it okey to add code into the POCO classes? Would that have any influence on how the database is generated from my classes?

    What about the service classes? I mean the UsersManager, ListingsManager and the likes... I am mainly using them for filtering the retrieved data and plan to add validation logic as well. The interface of my repositories is something like that:

    public interface IUsersRepository
    {
      IEnumerable<User> GetUsers();
      bool AddNewUser(User user);
      bool DeleteUser(User user);
      bool CommitChanges();
    }
    
    And these are actually the only methods I implement in the concrete repository class. Then in the service class I do something like GetUserById(), CreateUser()... etc. So you could be right, I probably do not really need the service classes, I could move that logic into the repository itself, but in that case the validation should happen in the POCO classes themselves, right?

    Tuesday, February 1, 2011 3:07 PM
  • Yes, I think Derek definitely does like refactoring code ;^)

    The bit that interested me was where you stick the bid object.

    I think I'd be inclined to have a list of recently-bidded-items on the user object with a pointer to each item rather than the item.

    A list of bids in each item.

    I'm thinking some hashtables might be necessary to make this thing fast enough if the volume is high.

    Tuesday, February 1, 2011 3:19 PM
  • Yep a bit of OCD there on the refactoring wouldn't you agree? great practise though.

     

    @Kassem

    Keep your services for sure mate if you think it makes sense, this was the code I would have liked to see if I had written it from scratch. Pinch of salt yeah. :)

    User user = userManager.GetUserById(userId);   // is absolutely fine

    In fact the code I had might not have worked in all honesty... the default constructor is gone when this is done.

    User user = new User(userId);  // user has no default constructor; cannot be serialised !

     

    I mean this is still alright to me...  :)

        public bool PlaceBid(int userId, int listingId, double amount)
        {
          User user = users.GetById(userId);
          Listing listing =  listings.FindById(listingId);

          bool validBid = ValidateBid(user, listing, amount);
          if (validBid)
              user.bidFor(listing, amount)
          return validBid;
        }

     

    "Is it ok to add code into the POCO classes? Would that have any influence on how the database is generated from my classes?"

    No idea Kassem but it really... really ... shouldn't be a problem!!!

     

     

    You see Kassem there are two schools of thought.

    There are some people that like objects that only contain data (POCO) and they develop 'services' to work with these objects. Then there are people who don't like this claiming that solutions like that aren't truely object orientated, that it results in less encapsulation (objects should be the only ones to change their internal state, not external services). 


    It seems your in the middle. Does XYZ() go into an objects 'service' or into the object itself?

     

    The refactoring I did was based on the second school of thought because it's the approach I think is correct but then again I am NOT using the Entity Framework.

    Need to stop in a bit, got somewhere to be.

    Think of the validation as an example. In one approach there is a validator service that can validate all POCO objects. In the other approach each object knows how to validate itself.

    Where should the validation logic go?

    Another example, adding a new user. In one approach there is a UserManager class with an AddNewUser() method. In other approach you create a new User object, set it's values, and call it's Save() method.

    What do you do?

    :D I'll check back later.


    …we each have more potential than we might ever presume to guess. (Blog: http://dsmyth.blogspot.com/)
    Tuesday, February 1, 2011 4:14 PM
  • I see what you mean there Derek. To be honest, I think I'm mixing and matching from both school of thoughts, because I'm lost though, not because I'm being innovative :D

    I actually prefer keeping responsibilities within the class itself! That makes much more sense to me. Moreover, creating service classes for each type or object is kind of an "overkill". I will try to move the GetUserById(userId) into the repository, or maybe create extension methods to do so - I'm a big fan of fluent interfaces. 

    But one more thing, as you can see, I have to add the same Bid object to both the User and Listing instances, I do not like that. Is there a better way of doing this? Below are my POCO classes:

      public class Listing
      {
        public int ID { get; set; }
        public string Title { get; set; }
        public string Description { get; set; }
        public float StartingPrice { get; set; }
        public float BidIncrement { get; set; }
        public float? RetailPrice { get; set; }
        public DateTime StartDate { get; set; }
        public DateTime EndDate { get; set; }
        public bool Published { get; set; }
        public virtual Address PickupAddress { get; set; }
        public virtual User User { get; set; }
        public virtual ICollection<Category> Categories { get; set; }
        public virtual ICollection<Comment> Comments { get; set; }
        public virtual ICollection<Image> Images { get; set; }
        public virtual ICollection<Bid> Bids { get; set; }
        public virtual ICollection<WatchList> WatchLists { get; set; }
        public virtual ICollection<ViewList> ViewLists { get; set; }
      }
    
      public class User 
      {
        public int ID { get; set; }
        public string NickName { get; set; }
        public string FirstName { get; set; }
        public string LastName { get; set; }
        public string Email { get; set; }
        public string Password { get; set; }
        public int Credits { get; set; }
        public int PromotionalCredits { get; set; }
        public string Telephone { get; set; }
        public string Mobile { get; set; }
        public int DefaultAddressId { get; set; }
        [ForeignKey("DefaultAddressId")]
        public virtual Address DefaultAddress { get; set; }
        public virtual ICollection<Address> Addresses { get; set; }
        public virtual ICollection<Role> Roles { get; set; }
        public virtual ICollection<Comment> Comments { get; set; }
        public virtual ICollection<Listing> Listings { get; set; }
        public virtual ICollection<Bid> Bids { get; set; }
        public virtual ICollection<CreditCard> CreditCard { get; set; }
        public virtual ICollection<Message> ReceivedMessages { get; set; }
        public virtual ICollection<Message> SentMessages { get; set; }
        public virtual WatchList WatchList { get; set; }
        public virtual ViewList ViewList { get; set; }
        [NotMapped]
        public string FullName
        {
          get
          {
            return FirstName + " " + LastName;
          }
        }
      }
    
      public class Bid
      {
        public int ID { get; set; }
        public float Amount { get; set; }
        public DateTime Date { get; set; }
        public virtual Listing Listing { get; set; }
        public virtual User User { get; set; }
      }
    
    Any suggestions? :)

    Tuesday, February 1, 2011 5:11 PM
  • Another thing... What about using extension methods on the User class... Something like that:

        public static bool PlaceBid(this User user, Item auction, float amount) 
        {
          Bid bid = new Bid
          {
            Amount = amount,
            Date = DateTime.Now,
          };
    
          try
          {
            user.Bids.Add(bid);
            user.Bids.Add(bid);
            return true;
          }
          catch (Exception ex)
          {
            //TODO: Log exception
            throw ex;
          }
        }
    

    Wednesday, February 2, 2011 5:05 PM
  • Hi Kassem, sorry for not replying yesterday.

    I'd say it's ok to mix and match from the two schools of thought as long as there is a) consistancy and b) a safety net.

    Consistency; if there is a user.Save() method then there should be a listing.Save() method as well.

    Safety net: Any choice made should be easily reversed... those difficult to make decisions are no longer critical.

     

    The code should be readable and testable at the end of the day.

    Everything in it's place; and that's what catches you out when mixing and matching. Depending on what approach you prefer generally dictates where stuff goes.

    What I would say is rather than developing services for each POCO object develop services that represent a piece of application functionality. In the case of your code the UserManager and ListingManager .... well they both provide the same service, or at least they are very similar, but for different object types. It's like a database service. There is also perhaps another service ... a bidding service.

    I don't know if I'm helping you out here or making matters worse.

     

    Answering posts in the architecture forum isn't the same as in other code based forums.

    There isn't usually a right answer with architecture, only opinions and points of view.

     

    Take your next question use extension methods or a partial class?

    For me I'd use a partial class since you have the source code but then again extension methods work too. Which one is better?

    Depends.

    I mean maybe a service can be provided to a class using a collection of extension methods the way LINQ adds a querying service to IEnumerable objects. Import the namespace import the service. That's quite a nice solution actually but is it better?

     

     


    …we each have more potential than we might ever presume to guess. (Blog: http://dsmyth.blogspot.com/)
    Thursday, February 3, 2011 8:30 AM
  • Thanks for your input Derek, you've definitely been of great help :)

    <quote>What I would say is rather than developing services for each POCO object develop services that represent a piece of application functionality. In the case of your code the UserManager and ListingManager .... well they both provide the same service, or at least they are very similar, but for different object types. It's like a database service. There is also perhaps another service ... a bidding service.</quote>

    I totally agree on this one. This is what I've been thinking of actually. I'm planning to move all of my "database service methods" into the repositories, because after all a repository is a service that makes it easy to query and update the database, at least that's how I understand it. So I've been searching for articles and blogs on the subject trying to find solutions to my issue and I found this article. I love the way he's using generic methods in his GenericRepository which makes it easy use the same repository instance to retrieve data about User, Items, Addresses... etc. Then I could inherit from this class and create specific repositories (UserRepository for example) to do the more specific queries... 

    I have a few other resources I need to read as well. Will report back with the outcome and will be glad to hear more thoughts from you. Thanks again :)

    Thursday, February 3, 2011 8:43 AM
  • Kassem,

    Here is an article that shows something quite interesting; maybe you have seen this already.

    http://blogs.hibernatingrhinos.com/nhibernate/archive/2008/10/08/the-repository-pattern.aspx

    It's quite similar to your situation (and the article you posted above) and the author has the same idea as yourself of moving the database methods into a repository class and then generalising the repository definitions. That's actually quite nice because you have the option for adding caching in the repository as well; as Andy mentioned above, if the database is hit a lot.

     'a repository is a service that makes it easy to query and update the database'.... yeah man that's it... I'd go with that. Spot on.

     

    Nice one.

    .


     


    …we each have more potential than we might ever presume to guess. (Blog: http://dsmyth.blogspot.com/)
    Thursday, February 3, 2011 11:13 AM
  • please do use entity framework 4.0 along asp.net mvc , use only Microsoft technologies. Please do not mix things , other wise the consequences in its all aspects will be very great. 
    PHIJO MP
    Thursday, February 3, 2011 1:03 PM
  • please do use entity framework 4.0 along asp.net mvc , use only Microsoft technologies. Please do not mix things , other wise the consequences in its all aspects will be very great. 
    PHIJO MP
    Thursday, February 3, 2011 1:03 PM
  • please do use entity framework 4.0 along asp.net mvc , use only Microsoft technologies. Please do not mix things , other wise the consequences in its all aspects will be very great. 
    PHIJO MP

     

    Of course you're free to recommend MVC and EF and qualify that but to suggest you have to use MS-only is utter rubbish. I'd recommend *you* take a good look at the alt.net community. Sorry if this is harsh, if you truly believe MS is the only way then start a new post and say why nServiceBus, nCQRS, nHibernate, OpenRasta, OpenWrap, Mono, etc, etc should not be used to give the community an opportunity to defend them.


    http://pdkm.spaces.live.com/
    Monday, February 14, 2011 7:05 AM