Good OO design (lack of friend modifier)

C

cody

Hi!

I have a class Customer which contains Contacts in a ContactsCollection.

class Customer
{
...

public ContactsCollection Contacts
{
get {...}
}
}

class Contact
{
...

Contact(Customer cust)
{
cust.Contacts.Add(this);
}

public void Delete()
{
cust.Contacts.Remove(this);
}
}

The problem now is that the add and remove methods of the
ContactsCollection are public so I cannot ensure that somebody else
tries to remove a contact without deleting it or trying to add a contact
twice and so on.

Is there a way to prevent this so that nobody comes to the idea to call
add/remove manually? I do not want that add/remove is visible outside
these 3 classes.

I know I can throw an exception if somebody tries, or make add/remove
internal but in my assembly are hundreds of classes so internal so it
doesn't really feel right to me.

I wonder how other people get around this problem as there should be
many programs out there with a similar constellation, I suppose .)
 
Q

quamaretto

cody said:
Hi!

I have a class Customer which contains Contacts in a ContactsCollection.

class Customer
{
...

public ContactsCollection Contacts
{
get {...}
}
}

class Contact
{
...

Contact(Customer cust)
{
cust.Contacts.Add(this);
}

public void Delete()
{
cust.Contacts.Remove(this);
}
}

The problem now is that the add and remove methods of the
ContactsCollection are public so I cannot ensure that somebody else
tries to remove a contact without deleting it or trying to add a contact
twice and so on.

Is there a way to prevent this so that nobody comes to the idea to call
add/remove manually? I do not want that add/remove is visible outside
these 3 classes.

I know I can throw an exception if somebody tries, or make add/remove
internal but in my assembly are hundreds of classes so internal so it
doesn't really feel right to me.

I wonder how other people get around this problem as there should be
many programs out there with a similar constellation, I suppose .)
Click to expand...

I find it strange that the Contact has a method to remove itself from
the collection. It would make more sense to move that functionality
into the customer class and call "cust.RemoveContact(this_contact)"
instead of "this_contact.Delete()" which does not seems suspicious.

After that, you could hide the contact collections itself and only
expose the bits of it necessary to outside objects (by writing a custom
proxy class with an indexer, by writing individual access methods, by
exposing it as an IEnumerable, whatever).
 
G

Greg Young

I agree with the the fact that the remove method should exist on the parent.

Another way of doing this is through the use of an event/delegate etc. This
would allow you to have a delete method on the contact which in turn raised
a "deleted" event. All of the containers that held the contact could then
remove it from their containers. I personally use this "pattern" in quite a
few places that require me to have the delete on the item as well as on the
parent.

Also since the only addition is a "Deleted" event it is unlikely that you
would even want to make this "Friend" ... it is useful functionality to
offer to all classes that deal with contacts.

Cheers,

Greg Young
MVP - C#
http://codebetter.com/blogs/gregyoung
 
B

Brian Gideon

Here is one possibility. There are several variations on this pattern
that may be more suitable for your situation, but I think this will get
you started.

public class Customer
{
private MyContractCollection contracts = new MyContractCollection();

public ContractCollection Contracts
{
get { return contracts; }
}

private MyContractCollection : ContractCollection
{
public void Add(...) { }

public void Remove(...) { }
}
}

public abstract class ContractCollection : ICollection
{
// Put methods and properties you want to expose to everyone here.
}

Brian
 
C

cody

You declared the MyContractCollection private inside the Customers class
so how will anybody from outside Delete a Contract and get it Removed
from the list?
 
C

cody

I find it strange that the Contact has a method to remove itself from
the collection. It would make more sense to move that functionality
into the customer class and call "cust.RemoveContact(this_contact)"
instead of "this_contact.Delete()" which does not seems suspicious.
Click to expand...

Let us assume I have a methods in Customer which are able to Delete
Contracts. So I would have all the logic of lots of entities in my
customer class:
DeleteContacts,DeleteContract,DeletePayment,DeleteConditions,DeleteAddress,DeletePricelist.
The logic of deleting a Contract for example should reside in the
Contract class, anything else is not good oo imo. Only the Contract
class knows in which Database Table it resides and how to deal with its
data. I don't want to expose this private knowledge to other classes.
 
C

cody

But using events is not typesafe.
You can attach any method on any event (if signature matches).

myContract.Deleted+=myContract.Delete;
myContract.Deleted+=myContract.PlaceOrder; // everything is possible

also, somebody from outside could remove the event handler.
 
B

Brian Gideon

cody said:
You declared the MyContractCollection private inside the Customers class
so how will anybody from outside Delete a Contract and get it Removed
from the list?
Click to expand...

I guess that's the point. Only the ContractCollection itself or the
Customer class can Add or Remove contracts. So instead of everyone
being able to do this you've limited it to only 2 classes. I was
thinking there would be some method on the Customer class that performs
all of the work necessary to delete a contract. That's what I thought
you were after.

But, after reading your other post I see where you are coming from now.
Why not use Greg's method but make the Deleted event internal so that
only classes in the same assembly can subscribe? If you have complete
control over the assembly then you'll be fine. Callers outside of the
assembly won't even see the deleted event. There's no reason you
should have to jump through hoops to protect the event from yourself.
Afterall, you coded it and should know how to use it properly.
 
C

cody

Brian said:
I guess that's the point. Only the ContractCollection itself or the
Customer class can Add or Remove contracts. So instead of everyone
being able to do this you've limited it to only 2 classes. I was
thinking there would be some method on the Customer class that performs
all of the work necessary to delete a contract. That's what I thought
you were after.

But, after reading your other post I see where you are coming from now.
Why not use Greg's method but make the Deleted event internal so that
only classes in the same assembly can subscribe? If you have complete
control over the assembly then you'll be fine. Callers outside of the
assembly won't even see the deleted event. There's no reason you
should have to jump through hoops to protect the event from yourself.
Afterall, you coded it and should know how to use it properly.
Click to expand...

Well, we have 100+ classes in our assemblies and Iam not the only one
who is coding classes into this assembly so internal is not of great use
as usual.

Additionally, suppose I would move the logic of deleting/removing a
contract from the contract class into the contractcollection or into the
customer class. So what if some code only has created a contract object
(without any customer) and now want to call delete on it? Do we now
always have to load the customer and all items in the contactcollection?

Maybe there is no perfect solution and we have to make drawback here.
Either we restrict Add/Remove to the assembly, allowing only 100+
classes to access it
Or we move logic into the Customer or the Collection where it doesn't
belong to (imo), and we are required to always have the whole hierarchy
loaded to delete a contact.
 
G

Greg Young

Or you use something along the lines of an event or an observer pattern on
the Contact object. You said that you didnt like events as they were not
strongly typed. An observer on the other hand would definately be strongly
typed.

I think another interesting point is your definition of a contract ... Can a
contract exist outside of the scope of a customer in your system? This seems
like something feasable but it really depends on your domain, I quite often
see people making such arguments for things like "OrderLineItems" in
relation to an "Order" which obviously is a non-existent situation as it
makes no sense to have a transient order line item in 99% of circumstances
(someone is buying something outside the context of a sale?). This question
needs to be asked and is discussed heavily in both DDD (Evans) and ADDDP
(Nilsson) which may be worth a read for you. By saying that Contract can
exist on its own you are defining in terms of DDD it to be an aggregate root
which gives it some special handling. I would recommend a read through DDD
as it formalizes many of these types of issues and discusses various
strategies to work around them.

Cheers,

Greg Young
MVP - C#
http://codebetter.com/blogs/gregyoung
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

OO Design Question 6
Generics; OO 4
Access Modifier Question 3
OO Design 3
Fundamental OO Question 3
OO Design and n-tier systems 4
OO design question... 7
Decentralized unit initialization 11

Top