Which class structure is more desirable?

Question

I'm not sure which of these two "patterns" is the best. Currently I use option A (in conjunction with a provider for implementing persistence), but I'm now erring towards B, especially in light of unit tests being able to use the "dependency injection" model.

Option A:

class ClassA
{
   ClassA() { }
   Save();
   static List<ClassA> GetClassAs();   
}

Option B:

class ClassA
{
   ClassA() { }
   Save();
}

class ClassARepository
{
    ClassARepository() { }
    List<ClassA> GetClassAs();    
}

I think what I'm asking is, is it good practice for a class to expose static methods that return collections of instances of itself?

Edit

There seems to be a general consensus that Option B is the better choice. Looks like I have plenty of refactoring ahead :S

Answer 1

You either divide completely persistence of the object from the data and logic or keep all in the one class. Move 'Save' to 'ClassARepository' or keep 'GetClassAs' inside 'ClassA'.

Answer 2

No, I wouldn't consider that bad practice as a hard and fast rule, though I'm assuming that this is for some kind of instance tracking or resource monitoring? If so, just remember that there will have to be logic behind the scenes to ensure that objects that would otherwise be out of scope don't stick around in your static collection only. I'm not sure which language you're using, but if it's C# you could consider storing it internally as a List<WeakReference>.

Answer 3

Option B looks a bit like the ActiveRecord pattern (I Assume the Save method in ClassA will use the ClassARepository ? ), which is good in some situations, but, if you have rather complex domain-model, I wouldn't use the 'ActiveREcord' pattern.

Instead, I would use such a model:

public class ClassA
{
   public int Id {get; private set;}
   public string Name {get; set;}
}

public class ClassARepository
{
    public ClassA Get( int id );

    public void Save( ClassA item );
}

Which means that all persistence related logic is put in the ClassARepository class, and ClassA has also no direct access to the repository.

Answer 4

Option B looks better because A mixes two ideas, two kinds of functionality. There's the actual function of the A class and the separate function of managing the collection of A's.

As the application grows it's also likely that you'll need additional functionality in your repository - findById, retrieving subsets, etc.etc. Shoehorning this into the A class will get messy.

And, as you've noted, option B is easier to test.

Answer 5

Option B is more flexible.

• It allows for the possibility to have multiple repositories.
• It allows the repository class to be subclassed.
• It allows you to pass the repository as an argument to another class or function instead of having that class or function be explicitly coupled to a global static variable.

Answer 6

I think what I'm asking is, is it good practice for a class to expose static methods that return collections of instances of itself?

Generally no - such collections are often singletons, and if they are (as in your example) then the usual reasons for singletons being considered an antipattern, as well as the reasons which static is considered harmful.