views:

219

answers:

8

Hi, what I mean by that is: I basically have a class that has too many properties and functions now. To remain performant and understandable, it needs to shrink somehow. But I still need all those properties and methods somewhere. It's like this right now:

class Apple

  float seedCount;
  ...
  ...about 25 variables and properties here.
  void Update() <-- a huge method that checks for each property and updates if so

In most cases the class needs almost none of those properties. In some cases in needs to be able to grow very selectively and gain a feature or lose a feature. The only solution I have come up with, is that I create a bunch of classes and place some properties in there. I only initialize this classes object when one of those properties is needed, otherwise it remains null.

class Apple

  Seed seed;

Many problems because of that: I constantly have to check for every single object and feature each frame. If the seed is not initialized I don't have to calculate anything for it. If it is, I have to. If I decided to put more than 1 property/feature into the Seed class, I need to check every single one of those aswell. It just gets more and more complicated. The problem I have is therefore, that I need granular control over all the features and can't split them intelligently into larger subclasses. Any form of subclass would just contain a bunch of properties that need to be checked and updated if wanted. I can't exactly create subclasses of Apple, because of the need for such high granular control. It would be madness to create as many classes as there are combinations of properties. My main goal: I want short code.

A: 

My main goal: I want short code.

Options:

  1. Rewrite all functions as static and create a class for each one.
  2. Rewrite your codebase in Perl.
  3. Remove all comments.
mcandre
Static methods is pretty much never a good solution.
Mattias Jakobsson
sry, I just edited the post to say that I can hardly create as many subclasses as there are combinations of properties. Also: I dont really have that many functions. I just have a huge update function and a lot of properties.
Blub
@Mattias He asked for short code, not good code.
mcandre
@mcandre, As I see it he asks for maintainable code. Making random stuff static certainly doesn't help for maintainability.
Mattias Jakobsson
Asking for maintainable code is not the same as asking for short code. Ask an ex-Perl programmer.
mcandre
@mcandre, Of course, but I think that is pretty much what he means. He wants to make that current implementation better (scalable, maintanable etc). Making stuff static certainly doesn't help there.
Mattias Jakobsson
True, I think it goes without saying that noone wants unmaintainable code. If ask for fast code, I probably don't want an answer like "rewrite your codebase in assembler". I hope you understand, but thanks anyway. Especially when I added a "c#" tag.
Blub
+4  A: 

It would be madness to create as many classes as there are combinations of properties.

Sounds like you might be looking for the Decorator Pattern. It's purpose is to make it easier to manage objects that can have many different combinations of properties without an exponentially growing heirarchy. You just have one small subclass for each property or behavior (not necessarily one C# property, just something you can group together) and then you can compose them together at runtime.

In your case, each Apple decorator class will override your Update method, and make the calculations necessary for its parts, and then call base.Update to pass it to the next in line.

Your final answer will heavily depend on exactly what your "Apple" really is.

Tesserex
it's like shop class: it has so many properties because it must be able to spit out so many different kinds of stuff depending on what the customer selects.
Blub
suppose I have 30 classes then, isn't there a huge performance hit when I want them all?
Blub
I just realized while implementing that the decorator doesn't solve the biggest issue: setting the properties. Accessing properties is a real pain, I would need to remember the actual type of the decorated object and cast each time I want to set a property.
Blub
Initializing such composited classes is near to impossible to do.
Blub
Ok, if it's a shop, are you saying that you are trying to have a "Shop with items 1 2 and 3 selected", a "Shop with items 2 4 and 7" selected", a "Shop with items 1, 2, 4, 5, 8 selected", etc? Is that what's going on?
Tesserex
Yes, you are right. Maybe a better analogy: a customer wants to buy a dish that has X, Y, and Z in it as ingredients. I can create a dish with this pattern like that: new X(new Y(new Z())). Now I got a "X" on my hands. How do I modifiy a property of Z when I only have the properties of X at my disposal? Only way: Each ingredient has a baseclass that implements all properties of all ingredients. This is incredibly wasteful however...... If I made a dish with 30 ingredients, each ingredient would contain all other properties needlessly.
Blub
The problem summarized: I need access from each ingredient to each and every other ingredients properties.
Blub
I'll mark this as answer again, because I don't think there is a better solution. Even multiple inheritance in c++ would not help, because I can hardly create objects that inherit from a dynamic range of classes. (not without some serious reflection magic)
Blub
+1  A: 

you may use a nested class in Apple class http://msdn.microsoft.com/en-us/library/ms173120(VS.80).aspx

Arseny
that doesn't help because it actually increases the amount of code
Blub
+1  A: 

I think the key thing here is that you are trying to hold everything in one class. Because of that, the class must be constantly checking what it has and what it doesn't. The solution is to create subclasses or decorators that already know whether or not they have a particular thing. Then they don't have to be checking it each time.

Because you have so many properties which may be combined in different ways, it sounds like the decorator solution is more up your alley.

jdmichal
+1  A: 

I think you're in the right path: composition. You compose your class with the other classes that are needed. But you also need to delegate responsibility accordingly. In your example, it's the Seed class that should be responsible for checking it's internal state, and Apple just delegates to it.

As for the optional features problem, maybe you can use null objects instead of null references. This way, you don't need to check for null everytime, and the code is more consistent.

Jordão
thanks for the hint with null objects, I was wondering how to overcome that aswell.
Blub
A: 

Maybe your methods are not were they are supposed to be?

If you separated the Seed class from the Apple class, why don't you move the methods that use the Seed information to the Seed class too?

If those methods need information on other Apple properties, you can pass it as a parameter.

By doing this, I guess you can eliminate the initialization checks...

This is a great book about how to solve this kind of problem:

Refactoring

mverardo
as I mentioned: every property needs (almost) every other property. There is no sense in coupling all the classes you suggest so strong together. It just makes interaction between the properties harder, makes for more and complicated code.
Blub
Sorry Blub, I think I didn't understand your problem.Could you give us an example that is closer to reality?Normally, when a class has too many properties, this means it has too many responsibilities.Maybe we can find better analogies to your class, and help you separate the properties (if it is the case)?
mverardo
Can you look at the comment I added to Tesserex' answer? It's a little hidden under a collapsed region I guess.
Blub
+1  A: 

I've been pondering this question for a bit and I've come up with an alternate solution. This may be a bit unorthodox and anti-object oriented, but if you're not faint of heart read on...

Building upon the Apple example: the Apple class can contain many properties, these properties which could be categorized into related groups. For the example I rolled with an Apple class with some properties related to the apple's seeds and others related to the apple's skin.

  1. Apple
    a. Seed
    a1. GetSeedCount
    a2. ...
    b. Skin
    b1. GetSkinColor
    b2. ...

I'm using a dictionary object to store all the apples properties.

I wrote extension methods to define accessors to the properties, using different classes to keep them separate and organized.

By using a dictionary for the properties, you can iterate through all properties stored thusfar at any point (if you have to check all of them, as it sounded like you needed in your update method). Unfortunately you lose strong typing of the data (at least in my sample I did because I'm using a Dictionary< string, string>. You could have separate dictionaries for every type needed, but that would require more plumbing code to route the property access to the correct dictionary.

Using extension methods to define accessors to the properties allows you to separate the code for each logical categories of properties. This keeps things organized into separate chunks of related logic.

Here is a sample I came up with to test how this would work, given with the standard warning that if you were to continue down this path robustification would be in order (validation, error handling, etc.).

Apple.cs

namespace ConsoleApplication1
{
    using System.Collections.Generic;
    using System.Text;

    public class Apple
    {
        // Define the set of valid properties for all apple objects.
        private static HashSet<string> AllowedProperties = new HashSet<string>(
            new string [] {
                "Color",
                "SeedCount"
            });

        // The main store for all properties
        private Dictionary<string, string> Properties = new Dictionary<string, string>();

        // Indexer for accessing properties
        // Access via the indexer should be restricted to the extension methods!
        // Unfortunately can't enforce this by making it private because then extension methods wouldn't be able to use it as they are now.
        public string this[string prop]
        {
            get
            {
                if (!AllowedProperties.Contains(prop))
                {
                    // throw exception
                }

                if (Properties.ContainsKey(prop))
                {
                    return this.Properties[prop];
                }
                else
                {
                    // TODO throw 'property unitialized' exeception || lookup & return default value for this property || etc.

                    // this return is here just to make the sample runable
                    return "0"; 
                }
            }

            set
            {
                if (!AllowedProperties.Contains(prop))
                {
                    // TODO throw 'invalid property' exception

                    // these assignments are here just to make the sample runable
                    prop = "INVALID";
                    value = "0";
                }

                this.Properties[prop] = value.ToString();
            }
        }

        public override string ToString()
        {
            StringBuilder sb = new StringBuilder();

            foreach (var kv in this.Properties)
            {
                sb.AppendFormat("{0}={1}\n", kv.Key, kv.Value);
            }

            return sb.ToString();
        }
    }
}

AppleExtensions.cs

namespace AppleExtensionMethods
{
    using System;
    using ConsoleApplication1;

   // Accessors for Seed Properties
    public static class Seed
    {
        public static float GetSeedCount(this Apple apple)
        {
            return Convert.ToSingle(apple["SeedCount"]);
        }

        public static void SetSeedCount(this Apple apple, string count)
        {
            apple["SeedCount"] = count;
        }
    }

   // Accessors for Skin Properties
    public static class Skin
    {
        public static string GetSkinColor(this Apple apple)
        {
            return apple["Color"];
        }

        public static void SetSkinColor(this Apple apple, string color)
        {
            apple["Color"] = ValidSkinColorOrDefault(apple, color);
        }

        private static string ValidSkinColorOrDefault(this Apple apple, string color)
        {
            switch (color.ToLower())
            {
                case "red":
                    return color;

                case "green":
                    return color;

                default:
                    return "rotten brown";
            }
        }
    }
}

Here is a test drive:

Program.cs

namespace ConsoleApplication1
{
    using System;
    using AppleExtensionMethods;

    class Program
    {
        static void Main(string[] args)
        {
            Apple apple = new Apple();

            apple.SetSkinColor("Red");
            apple.SetSeedCount("8");

            Console.WriteLine("My apple is {0} and has {1} seed(s)\r\n", apple.GetSkinColor(), apple.GetSeedCount());

            apple.SetSkinColor("green");
            apple.SetSeedCount("4");

            Console.WriteLine("Now my apple is {0} and has {1} seed(s)\r\n", apple.GetSkinColor(), apple.GetSeedCount());

            apple.SetSkinColor("blue");
            apple.SetSeedCount("0");

            Console.WriteLine("Now my apple is {0} and has {1} seed(s)\r\n", apple.GetSkinColor(), apple.GetSeedCount());

            apple.SetSkinColor("yellow");
            apple.SetSeedCount("15");

            Console.WriteLine(apple.ToString());

            // Unfortunatly there is nothing stopping users of the class from doing something like that shown below.
            // This would be bad because it bypasses any behavior that you have defined in the get/set functions defined
            // as extension methods.
            // One thing in your favor here is it is inconvenient for user of the class to find the valid property names as
            // they'd have to go look at the apple class. It's much easier (from a lazy programmer standpoint) to use the
            // extension methods as they show up in intellisense :) However, relying on lazy programming does not a contract make.
            // There would have to be an agreed upon contract at the user of the class level that states, 
            //  "I will never use the indexer and always use the extension methods!"
            apple["Color"] = "don't panic";
            apple["SeedCount"] = "on second thought...";

            Console.WriteLine(apple.ToString());
        }
    }
}

Addressing your comment from 7/11 (the date, not the store) :)
In the sample code you provided, there is a comment that states:

"As you can see, I can't call BasicBroodmother methods on "monster"

You realize you could do something like this at that point:

BasicBroodmother bm = monster as BasicBroodmother;
if (bm != null)
{
    bm.Eat();
}

There isn't much meat to your code, (I understand it was just an example), but when I look at it I get the feeling that you should be able to improve the design. My immediate thought was having an abstract class for broodmother which would contain default implementations of any attributes/actions that are common to all broodmothers. Then specialized broodmothers, like the magical broodmother, would contain any specialized attributes/actions specific to the magical broodmother, but also inherit from the abstract class and if necessary override the nessecary base attributes/actions.

I would take a look at the Strategy pattern for the design of the actions so that the actions (i.e. behaviours like eat, spawn, attack) can be swappable based the type of monster.

[edit 7/13]
Don't have time to go into details right now (need sleep), but I put together some sample code showing a different approach.

The code consists of:

  • Broodfather.cs - abstract class filled with all things common to different Broodfathers "types."
  • BasicBroodFather.cs - concrete class that inherits from Broodfather.
  • BroodfatherDecorator.cs - abstract class to be inherited by all Broodfather decorators.
  • MagicalBroodfather.cs - this class decorates/wraps a Broodfather with "magic"
  • BloodthirstyBroodfather.cs - this class decorates/wraps a Broodfather with "bloodthirst"
  • program.cs - demonstrates two examples: The first starts with a basic broodfather that gets wrapped by magic then by bloodthirst. The second starts with a basic broodfather and wraps it in the other order bloodthirst, then magic.
Robert
Nice idea Robert. I remember I did have similar problem and ended up with a similar construct. And yes, the biggest problem was type safety (had to put everything as a string). +1
Adrian Godong
well this only works well with properties that can be converted from and to strings easily. Converting all the time can be very bad for performance aswell. Just think about how much work it would be to convert a 4x4 matrix all the time. Or a class with a lot of properties. Problems like this would be easy with reflection. I could just loop over all properties of all decorater classes programmatically. But I wont do that either because of performance.
Blub
@Blub True, which is why I mentioned that one could implement type specific dictionaries and some "routing" code to control which dictionary to use. This could be implemented by using an additional "routing dictionary" which would have the property name as it's key and the strongly typed "property dictionary" as it's value. The indexer would need to be typed as object, instead of string as it is now. Then in the get/set of the indexer you'd need to look up the property name in the routing dictionary to get the appropriate strongly typed dictionary to retrieve a value from or set a value to.
Robert
@your 7/11 edit:No, I can't. At least not with the effect I want. All actual variables are located in the bottom-most decorater. In my example every call to anything is routed downwards until it is overriden by some decorator. If I call bm.Eat() it will call the Eat() method of the BasicBroodmother base class of MagicalBroodmother. However, the actual implementation and overriden method of Eat() lies in monster.decorater, which is also a BasicBroodmother.
Blub
Your Eat() call will not throw any exception BUT will possibly use variables that are not used or initialized. I would call this undefined behaviour in the pattern. Your suggestion to use an abstract class with common implementations is basically the same as my BasicBroodmother. It will override all common methods provided by the Mother class. The MagicalBroodmother already is designed to override specific methods of Mother that are specific to a MagicalBroodmother.
Blub
To clarify: I route every call to the bottom-most decorator because otherwise these two statements would result in objects that do completely different things:1. new BloodthirstyBroodmother(new MagicalBroodmother(new BasicBroodmother())2. new MagicalBroodmother(new BloodthirstyBroodmother(new BasicBroodmother())
Blub
@Blub: The way your implementing decorator is confusing to me. You're starting out with an instance of Mother, which is a decorator. I would expect you to start out with an instance of a class that isn't a decorator. This instance could then be "decorated/wrapped" by decorator objects ad infinitum. I have a sample to show how I would have approached it, I'll edit my answer above with more details.
Robert
Your solution certainly looks simpler, but I think you forgot to tackle the problem with property access. When I have a new BloodthirstyBroodfather(new MagicalBroodfather()) .. I cannot access MagicalDamage. I also can't cast to MagicalBroodfather. You could argue that I should simply take .father and cast that object. But this is impossibly hard and convoluted to do when using e.g ~30 decorators, and I want to set a property of some decorator in the middle. How navigate? Changing properties is very important, the magical damage might need to increase or decrease depending on location.
Blub
Just had an idea: One solution could be to override the implicit or explicit casting operator in c#, and iterate over all decorators and return the one that was asked for. This would be nice and short code, but I fear for performance. Casting and iterating over an array of 30 objects each time anything is requested or wanting to be set is more than frightening. (on the other hand...I suppose I do almost the same thing already in my Broodmother solution. When a property is requested, it steps through each decorator until it finds it overriden)
Blub
Hah! I think I know a better solution. It's a little ugly, but has a performance improvement: create an array of all decorators and create an enum for each one. Then, when I want to call .MagicalDamage, I don't need to run through anything, it's a simple indexing call.Example: (father as MagicalBroodfather).MagicalDamage = 4;This will call the overriden explicit operator, which looks like this:operator(ClassToCastTo){ return decorators[ClassToCastTo.ToString()];}It's not too cost intensive to create an array each time I want a new decorator.Would love to hear your input on this.
Blub
This answer was getting a little crowded so see my new answer.
Robert
+1  A: 

After reviewing your comments and samples in my other answer, I've thought about the Decorator pattern and how it was being used vs how you want things to work. I've come to the conclusion that Decorator is not right for this purpose. I'm thinking Strategy instead. I have modified the previous sample code for you to take a look at.

I've gotten rid of the decorators altogether. The Broodfather abstract class remains. It has two additional properties an IBroodfatherMagicAbility and an IBroodfatherBloodthirstAbility. This two properties will allow you to access the different attributes that pertain to those abilities, however the key to this all is that the strategy for implementing the abilities can change at runtime (see Strategy pattern).

There are two classes each that implement a "strategy" for both bloodthrist and magic.

  • IBroodfatherBloodthirstAbility.cs - this is the interface that all "bloodthirst strategies" must implement.
  • BroodfatherNonBloodThristy.cs - class that implements the attributes for non-bloodthirsty.
  • BroodfatherBloodThristy.cs - class that implements the attributes for bloodthirsty.

  • IBroodfatherMagicAbility.cs - this is the interface that all "magical strategies" must implement.

  • BroodfatherNonMagical.cs - class that implements a strategy for non-magical.
  • BroodfatherMagical.cs - class that implements a strategy for magical.

  • BasicBroodfather.cs - this is similar to the previous example, except that now when an instance is created the magic and bloodthrist properties get set to new instances of the non-magical and non-bloodthristy strategy objects.

  • Program.cs is the driver that shows the classes and how the different strategies can get swapped in and out at runtime.

I think you'll find that more suited to how you wanted things to work.

Robert
Well this adds a lot of code. A new class just to show that the extension is not used? To be honest, I think I'm going in circles. Everytime I consider composition, a million things that are bad about it occur to me. But the same is with decorators. Wich leads me to believe that I need to define the problem better...It seems I can't have easy access to each property + have a lot of them + small classes. When I have time, I will start programming the game around this sample. Right now, either solution could work, and I implemented the decorator one at work.
Blub
The real problem is probably how I could possibly combine all of the properties dynamically into some sensible output. Suppose I have 30 different extensions to my broodfather. Once he attacks, those extensions need to work together somehow. He shouldn't throw a fireball if he has a better extension. Well, as I said: I'm much too vague and want too much. As soon as I start with the actual game, I will know more. Still, +1 for making me think.
Blub