0

I want to move a bunch of similar methods to an external class. The class is initialized with the original class instance. From there I can access it either by property (persistent instance) or by method (new instance each time).

Which approach is favorable? In a real system I will have many of these classes holding many methods. I would like to hear your opinions and knowledge.

EXAMPLE:

I have a class, let's call it Zoo, and in my Zoo class I hold Lists of animals.

(These animals have no common properties or methods where an interface or inheritance would be applicable, this is purely to paint an example.)

public class Zoo
{
    public List<Monkey> MonkeyList;
    public List<Lion> LionList;
    public List<Turtle> TurtleList;

    public Zoo()
    {
        MonkeyList = new List<Monkey>();
        LionList = new List<Lion>();
        TurtleList = new List<Turtle>();
    }
}

Now I want to add animals to my Zoo, so I create some Add methods.

Instead of putting them directly to my Zoo class, I add them to a separate class and pass my Zoo instance, like so:

public class ZooAdd
{
    private Zoo _zoo;

    public ZooAdd(Zoo zoo) 
    {
        _zoo = zoo;
    }

    public void Monkey(Monkey monkey)
    {
        _zoo.MonkeyList.Add(monkey);
    }

    public void Lion(Lion lion)
    {
        _zoo.LionList.Add(lion);
    }

    public void Turtle(Turtle turtle)
    {
        _zoo.TurtleList.Add(turtle);
    }       
}

Now I need to add the animals, but faced with two options, unsure which is preferable:

Option 1: I create a property

public class Zoo
{
    public ZooAdd Add;

    public Zoo
    {
        Add = new ZooAdd(this);
    }
}

...

Zoo zoo = new Zoo();
Monkey monkey = new Monkey();

zoo.Add.Monkey(monkey);

Option 2: I create a method returning an instance (Factory pattern?)

public class Zoo
{
    public ZooAdd Add()
    {
        return new ZooAdd(this);
    }
}

...

Zoo zoo = new Zoo();
Monkey monkey = new Monkey();

zoo.Add().Monkey(monkey);

Both approaches satisfy the requirements and have little difference in appearance.

I believe that the second option may prove more useful for memory efficiency as the object created through the method is presumably eaten up by the GC after it's used but I have no concrete knowledge regarding that statement.

Please, lend me your wisdom.

EDIT:

You've all raised some justified concerns about my approach and I agree that it can be improved. I will be revising the structure of my classes with your suggestions and assistance in mind. Thank you all for your answers.

Improve this question
6
  • ZooAdd uses almost no memory, so don't worry about Option 2. Commented Oct 26, 2018 at 15:41
  • Are you implying that Option 1 is better? Commented Oct 26, 2018 at 16:05
  • 1
    The decision should be made based on "correctness", not "save 16 bytes". If, in a multi-threaded code, ZooAdd can handle multiple requests at the same time, go for Option 1. If ZooAdd has a lot of complex internal state and would have trouble adding multiple things at the same time, has transactions, etc, you'd want to lean towards Option 2. Commented Oct 26, 2018 at 22:53
  • 2
    "In a real system I will have many of these classes holding many methods. I would like to hear your opinions and knowledge." This doesn't sound smart. You should design a better system. Commented Oct 27, 2018 at 7:14
  • Can this problem not be solved by using a List<Animal>? (assuming all mentioned animals inherit from a base Animal class) Do you have an actual need to separate the animals in separate lists? Commented Oct 29, 2018 at 8:15

4 Answers 4

Reset to default
5

Can I ask why you are trying to do this? It is hard to give you guidance without knowing the motivation. But in both options you are creating an object and then throwing it away. Both options are...not optimal.

If your motivation is to separate the code, there are partial classes in some languages. You could also make a base class that provides all the add methods.

Some unsolicited advice though...methods should be verbs, action words. Monkey is not a verb. What action is taken when I call Monkey? If you are in a language that supports it, I'd just overload Add with the different animal types. Otherwise AddAnimalXXX would be a better method name as it describes what action the method is taking. Self documenting code FTW! Properties should not have side effects, so Add is a bad property in Option 1.

Improve this answer
5
  • Thanks for the guidance, a partial class would be much more suitable in this case. My main goal is just to organise the code by separating and grouping methods so this works perfectly. Commented Oct 28, 2018 at 0:00
  • 1
    a partial class isnt a great way to seperate code. ok you now have 2 files, but you still just have one class doing everything Commented Oct 28, 2018 at 15:13
  • 1
    methods should be verbs, action words. Monkey is not a verb. What action is taken when I call Monkey? I fully agree with your suggestion. However, in OP's defense, the method was meant to be used after .Add. (which is either a class name or factory method name). I don't quite agree with OP's idea of using the Add class/factory method, but I do see why he then would avoid a redundant second "add" in the subsequent method name. Commented Oct 29, 2018 at 8:17
  • @Ewan: Partial classes can be used in a bad way but that doesn't mean partial classes are bad or shouldn't be used. Yes, partial classes can be used to split a monolith class into multiple pieces, without actually solving the issue of having a monolith class. However, it's important to remember that having a monolith class is the actual issue; having a partial class is not an issue in and of itself - it is simply "unethically" used to hide the monolith from sight. Commented Oct 29, 2018 at 13:10
  • 1
    @flater I didnt say any of that, but you are right, I do think partial classes are always bad. When a question about them comes up, remind me and I will put a long answer detailing why I am totally right and you are totally wrong and we can have a dance off to decide what the correct answer going forward will be Commented Oct 29, 2018 at 18:02
2

In your example there isnt much difference, but I generally find option 2 hard to work with.

Its becomes most obvious when the object has properties. eg

Zoo.Add().BannanasIncluded = 2

Doesnt work, so you are forced to make it immutable

Zoo.Add(bannanasIncuded:2).Monkey()

Also, how do I know not to make my own adder?

var x = new ZooAdder(myZoo)

or keep a reference to one after its been used?

var x = Zoo.Add(bannanasIncuded:2)
x.Monkey()
x.bannanasIncuded = 3 //why cant I do this??!?
x.Monkey()

Or just use the field directly?

Zoo.MonkeyList.Add(new Monkey())

Also from a performance standpoint newing and collecting millions of adder objects isnt great.

Secondly, I guess you are creating these classes to add a fluent interface rather than add functionality?

While thats fine and some people love them, its not essential to the class and it would be better to make it a seperate dll if possible.

Perhaps you could make it an extension method instead. Then I can add monkeys without any monkey business.

Improve this answer
1
  • Although the other answer gave me the solution I needed I appreciate your details about the second option being more prone to issues. This was the information I was looking for initially. Commented Oct 28, 2018 at 0:07
2

If you go with ZooAdd aproach you'll violate Open Close principle: while adding new animal classes to Zoo you would also need to add new methods to ZooAdd. I don't fully understand what you're trying to achieve, but IMO ZooAdd is not the best way to solve your problem. If you go that rote you will need to maintain additionall class and it's tests, and the only gain would be syntatic sugar in form of:

zoo.Add.Monkey(monkey)

which can be seen as violation of Law of Demeter and can make you some more trouble in the future.

While assigning responsibilites in oo languages it's good to use Information Expert as a guidance and while designing messages calls between objects it's good to remember about Law of Demeter.

Imo you should go with plane and simple: 

public class Zoo
{
    public List<Monkey> Monkeys;
    public List<Lion> Lions;
    public List<Turtle> Turtles;

    public Zoo()
    {
        Monkeys = new List<Monkey>();
        Lions = new List<Lion>();
        Turtles = new List<Turtle>();
    }
}

and then you can:

Zoo zoo = new Zoo();
Monkey monkey = new Monkey();

zoo.Monkeys.Add(monkey);
Improve this answer
1
  • And more important: he is violating the principle of encapsulation - which was the starting point of Object Orientation. The Lists must be hidden from a user of the zoo class (give him an IEnumerable only), but methods for adding/removing animals may be provided via an extra interface. Commented Nov 5, 2018 at 9:25
1

Lots of repetition of code implies to me that there's might be an abstraction missing.

My initial thought is to have:

  • An Animal base class
  • A single List<Animal> in Zoo
  • A single Add(Animal animal) method in Zoo
  • Inherit off the Animal base class for Monkey, Lion, and any other animals you might want to add in future.

Alternatively, something I've done myself in the past is to use something like the Text Template Transformation Toolkit (T4) to generate repetitive code, and to put non-generated code in a partial class. I generally only use that when repetition is unavoidable.

Improve this answer
2
  • The example code I posted does not accurately represent my system in the sense that each class is unique in their usage and properties. Grouping them together using an interface or inheritance would not work in this instance. My apologies for the misunderstanding. Commented Oct 29, 2018 at 8:36
  • 1
    A base class won't work, but an interface might - given that you can add interfaces to any class that implement their signatures. It might still be possible to have a single Add function - you could potentially use reflection to find out what the object type is, look up an appropriate list in Zoo using reflection or maybe a dictionary, and then add the given object to the correct list. Commented Oct 29, 2018 at 8:46

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.