Is this bad oop design?

Question

I have class called Chicken and in Chicken I have some methods, so in another class where I instantiate and call methods on Chicken, I might do something like this:

Chicken chicken = new Chicken("Name","Description")


public void UpdateChicken(Chicken chicken)
{ 
   chicken.Update(chicken);
}

Is the above fine or does it present problems, if so, is it better to have another class, such as ChickenCalculations and do something like:

public void UpdateChick(Chicken chicken)
{
    ChickenCalculations.Update(chicken);
}

Here is an implementation:

Chicken chicken = new Chicken("Bob","Coolest Chicken", 4, 123, 5, 388, true, false, true);

Chicken anotherChicken = new Chicken()
anotherChicken.Update(chicken);
chicken.Update(chicken)

Here is a more practical example instead of using a Chicken:

public class AirlineBooking
{
    int BookingId {get;set;}
    string Name {get;set;}
    string Description {get;set;}
    decimal Price {get;set;}
    decimal Tax {get;set;}
    string seat {get;set;}
    bool IsActive {get;set;}
    bool IsCanceld {get;set;}


    public AirlineBooking(string name, string description, decimal price, 
                          decimal tax, string seat, bool isActive, bool isCanceled)
    {
        Name = name;
        Description = description;
        Price = price;
        Tax = tax;
        Seat = seat;
        IsActive = isActive;
        IsCanceled = isCanceled;
    }

    public Update(AirlineBooking airlineBooking, int id)
    {
          //Call stored proc here to update booking by id
    }

    public class BookingSystem
    {
       //Create new booking
       AirlineBooking booking = new AirlineBooking("ticket-1",
                                                   "desc",150.2,22.0,
                                                   "22A",true, false);

       //Change properties and update.
       booking.Name ="ticket-2";
       booking.Description = "desc2";
       booking.Price = 200.52;
       booking.Tax = 38.50;

       public void UpdateBooking(AirlineBooking booking, int id)
       {
            /* This is the meat of the question, should the passed in booking to
               update itself or should I have a Service Class , such as
               AirlineBookingOperations with an update method. */
            booking.Update(booking,id);
       }
    }
}

Slight

Why do you pass chicken itself to the Update function? Isn't chicken.Update() sufficient? – Diathermic

What's wrong with chicken.Update()? Btw you may want to provide a more meaningful example. It's hard to discuss design decisions when you're in a nonsensical domain. – Charming

I think the example is fine, I am just trying to find out, if it is fine if it is perfectly ok for one class to update itself. – Slight

@Allessandro, I had that in might as well. The reason why I was passing Chicken, was that if I created a chicken object outside and set properties, I can pass it in an update it. Did what I said made sense? – Slight

@Xaisoft: but what does it mean to update a chicken? Without knowing the meaning of that, it's hard to say whether it's okay for a chicken to update itself. – Charming

It could be fine, but in this case - without more informations about what you're trying to do - seems absolutely unnecessary. :) – Diathermic

The most important thing to remember is not to update your chickens before they hatch. – Pants

@Xaisoft: if I understand, you would like to update a chicken using data coming from another chicken. – Diathermic

I added some code to the post, hopefully it clears things up a bit. – Slight

Also notice that Chick takes quite a bit of paramters, that is why I was passing in a CHicken object instead of all the parameters to update. – Slight

I wonder what goes on in ChickenCalculations. – Tasia

Pardon me if I'm being annoying, but you still haven't told us what Update is supposed to do. What happens in the last example to anotherChicken and to chicken? – Charming

+1 for a question with a useful example that only consists of chickens that can be used to calculate! – Nsf

@MartinHo, Update just goes in and updates all the properties of Chicken, such as Name, Age, If the chicken died, etc. That's all – Slight

@halo, I just called it ChickenCalculations, it could have been called ChickenMethods or ChickenServices. – Slight

So, it is essentially a shallow-copy / clone? – Inoculation

So, you are passing in an object that you have already updated to itself, so that you can update it? – Abiding

I updated the post to have a more practical example. – Slight

plif.courageunfettered.com/archive/wc072.gif – Casiecasilda

Watch this video and redo what you have done: viddler.com/explore/GregYoung/videos/8 – Muhammadan

Answer 1

Why isn't the UpdateChicken function a member of the Chicken class?

That way, you wouldn't have to pass in an instance of a Chicken object, but rather just call the Update method on an existing instance:

Chicken chicken = new Chicken("Name", "Description");
chicken.Update();

It's generally best to encapsulate all the methods that operate on a particular class inside of that class, rather than splitting them up into a separate "helper" class. Let them chickens manage themselves!

Pants

"Why isn't the UpdateChicken function a member of the Chicken class": probably, a pig wants to update a chicken? :) – Toiletry

@khachik: Well, sure. But then you would pass the pig an instance of the Chicken class, and he could call the Update method himself! – Pants

What if Chicken has a ton of parameters? – Slight

@Cody: Hmm, maybe pigs here are somewhat democratic, although they are pretending despots. Just joking. – Toiletry

@Xaisoft: Do you mean the constructor accepts a ton of parameters? I'm not sure how/why that's relevant. You only call the constructor once, and then just keep an instance of the class you create. If the Update method accepts a bunch of parameters, then I'd be suspicious of your design. Obviously there are cases where this would make sense, but the better idea would be to manage all those properties internally, inside of the Chicken class. Just like a real chicken, you feed it food and out comes an egg. You don't have to worry about the internal mechanisms and workings of the chicken. – Pants

@Cody, putting all behavior in one class can ultimately lead to very large classes with chunks of related behavior that are mutually independent of one another. Solving that problem is one of the reasons for the Visitor pattern. Another example are WCF classes; it is common to make the data classes completely light-weight and keep the operations on the data classes separate (in the services). This allows you to group related behavior in its own class (and potentially in its own assembly). – Reggi

@Kirk: Yes, I said it was "generally" best to do this. I don't believe in "rules" or anything absolute when it comes to design patterns. I also made the point that everything directly related to Chicken objects should be inside of the Chicken class. I still think that's true. Breaking those functions off into a helper class is rarely a good idea. I prefer to keep my models simple and intuitive, rather than try to wrap my mind around the newest pattern/model of the day. – Pants

Well said that man. Reading chicken.Update(chicken); gave me vertigo. – Abiding

Answer 2

The whole idea of Object Oriented Programing is to think of objects as able to act upon themselves.

So you should just use chicken.Update() to update a chicken.

Euboea

Answer 3

I'm going to use your AirlineBooking class as an example because a lot of people seem to have confused themselves over the Chicken example.

Some introduction:

The Single responsibility principle states that an object should have a single responsibility and that it should only concern itself with things narow aligned with that responsibility. For example a TaxCalculator should only be responsible for calculating tax and not, for example, with converting currency - this is the job of the CurrencyConverter.

This is often a really good idea, as it means that your application is structured into chunks of code, each one with a single responsibility making it easier to understand and safer to change. Another way of putting this is that a class or module should have one and only one reason to change, for example "The way we calculate tax has changed", or "The way we convert currency has changed".

---

The questions you need to ask yourself are:

• What is the responsibility of AirlineBooking?
• Is updating an airline booking part of that responsibility?

For example in this case I would say that the responsibility of AirlineBooking is "Encapsulating an airline booking", and that updating an airline booking is in fact the responsibility of the booking system, not AirlineBooking.

Altertnaively, another way of thinking about this is that if I put the Update method on AirlineBooking this would mean that:

• If the booking system changes to use a web service rather than a stored procedure then the AirlineBooking class needs to change.
• If the encapsulation of an airline booking changes (maybe it is possible to suspend a booking, or the name of the airline is now recorded) then AirlineBooking needs to change.

I.e. AirlineBooking now has many different reasons to change and so it shouldn't also be responsible for "Updating"

---

In short, I'd probably do this:

public class AirlineBooking
{
    public int BookingId {get;set;}
    /* Other properties */
}

public class BookingSystem
{
    public void UpdateBooking(AirlineBooking booking, int id)
    {
        // Call your SP here.
    }
}

---

The reason why you should ask yourself these questions is because it does depends on what AirlineBooking is used for in your application.

For example, if AirlineBooking is "aware" (i.e. has a reference to) the booking system then you could add a "helper" method, like this:

public class AirlineBooking
{
    public void Update(int id)
    {
        this.bookingSystem.UpdateBooking(this, id);
    }
}

Monopolize

I agree with you. I had it like this before, but then changed it. Seems like a lot of people could not concentrate with chicken as an example. Thanks for the post. I will read it over again. – Slight

Answer 4

Why don't you give your Chicken class a method "Update(some parameters...)"? Then, you can just instanciate a chicken by

Chicken chicken = new Chicken("Name", "descr");

and update by:

chicken.Update(myparameters..);

EDIT

public class Chicken
{
  public Chicken(string name, string description)
  {
     this.Name = name;
     this.Description = description;
  }

  public string Name { get; set; }
  public string Description { get; set; }

  // Fill in all the other properties!

  public int EggsDroppedInLife { get; set; }
}

And now you can use your chicken class the following way:

Chicken chicken = new Chicken("Harry", "Nice chick");
chicken.NumberOfEggs = 123;
chicken.Description = "Oh no, it's actually not nice.";
// ... change all the properties as you want

Guacharo

What if Chicken has tons of parameters? – Slight

@Slight - Generally, a method should not have tons of parameters. I would say up to 4-5 parameters is fine, everything more is questionable. But you can have Properties! If you chick has 10 properties, you can set them one by one. Do you know how Properties work? – Guacharo

@Slight if Chicken has tons of parameters you can use overloading and default parameters. – Lane

Sometimes I get confused on whether to pass in parameters or update the properties in the Update method.Is what you are saying is to update the properties in the Chicken Update method. – Slight

@Slight - Wait. I will edit my post and do a proper example. – Guacharo

@Christian: how can something from outside set eggsDroppedInLife to a chicken? – Toiletry

@Toiletry - because there is a Property exposing the private int eggsDroppedInLife. Note that from outside you cannot use "eggsDroppedInLife" but you have to use "EggsDroppedInLife" with capitol 'E', because that's the name of the property. Using those properties helps avoiding an 'Update-Method' with tons of parameters. Because maybe you only want to update just one parameter. – Guacharo

How do I decide whether to pass in parameter to update a property of Chicken or Update the properties directly without passing in parameters – Slight

@Christian: So you have public int EggsDroppedInLife with public set. This means that someone can set EggsDroppedInLife for a chicken. Not technically, but logically, it is a lack of encapsulation. – Toiletry

@Slight - Well you should use an Update-Method with parameters if you always want to update all the parameters AND there are only a few of them. You should rather use Properties if you have lots of things to update and you would like to sometimes update a subset, possibly just one. – Guacharo

@Toiletry - you are right, logically this does not make sense, only the chicken can update its value for EggsDropped. But I was just making an example and couldn't come up with parameter names, so I just picked some random stuff. – Guacharo

Could you please answer #9511637 ? – Quianaquibble

Answer 5

Objects should encapsulate functionality. Functionality should be passed in to give the encapsulating object flexibility.

So, if you're saving the chicken, you should pass in the repository functionality. If you have chicken calculations, and they're prone to change, it should be passed in as well.

class Chicken
{
   IChickenCalculations ChickenCalculations;
   IChickenRepository ChickenRepository;
   Chicken(IChickenCalculations chickenCalculations, IChickenRepository chickenRepository)
   {
       ChickenCalculations = chickenCalculations;
       ChickenRepository = chickenRepository ;
   }

   Calculate()
   {
       ChickenCalculations.Calculate(this);
   }
   Update()
   {
       ChickenRepository.Update(this);
   }
}

Note that how in this example the chicken is able to both perform calculations on itself and persist itself, without having any knowledge of how to perform calculations or persisting things (after all, it's only a chicken).

Tasia

Answer 6

While I realize there is no Chicken there might be an Update method on your real object, right?

I think you should try introduce something else than "update" in terms of language. There is no way to really understand what update does. Does it just update the "data" in the Chicken? In that case, what data? And also, should you be allowed to update an instance of Chicken like that?

I would rather see stuff like

chicken.CanFly = false;
if(chicken.CanFly)  // inherited from Bird :)
    ckicken.FlyTo(point);
else
    chicken.WalkTo(point);

Here is a pretty interesting exercise in OOP: http://milano-xpug.pbworks.com/f/10080616-extreme-oop.pdf

Helmet

'...understand what update does'. must give it go faster stripes - could consider naming it PimpMyChick? – Inoculation

Haha.. I'm just really into the Greg Youngish 'introduce more verbs' thing. Update is pretty general though. What happens when you update a chicken? Is it the entire chicken that gets updated or does it get new wings? I think this might benefit from some context though. – Muhammadan

No, nothing other than the stripes and a big-screen T.V (on a chain.) ;) – Inoculation

Answer 7

For multithreaded environment, having a seperate class like ChickenCalculations suits better. When you need to perform few other steps besides what chicken.Update() does, you can do that with ChickenCalculations class. So if multiple classes that instantiate and call methods on Chicken does not have to worry about the same things that ChickenCalculations class is taking care of.

Subsolar