0

I currently have a number of classes (~20) that all do the same thing (abstractly), namely, generate an instance of a particular class from an xml file.

The way that they use the content of the xml file varies quite a bit, and the data gets stored into different substructures (fields, groups, etc) that are shared across the classes, so I took all of the parsing and field information out of the files, and created a helper class (as is the final constructed type)

All of the above classes and methods are static, as they have no need to contain any state.

I then wanted to have all the classes derive from one common class so that I am not referencing a seemingly unrelated class constantly, only to discover that inheritance and static do not work together well.

What is the appropriate way of setting this up? Its bascially a large set of factory methods for a type that require no state.

Update: The control flow is currently as follows:

  1. Pick one of ~20 factory methods
  2. Read in an XML file (all tags, attributes are the same across the files, but the files are distict across the methods).
  3. Convert the content in the XML files to a class that has some data structures as members (List<Field>, List<Group>, Dictionary<string,string> etc). These data structures use types that are nested in the helper.
  4. Add these collections to the uninitialized object.
  5. Return the object to caller.

Right now, the 20 factory methods are thier own static classes, and all return objects of the same type, as even though there might be some semantic difference between the "types" all of the memebers and functions are the same. The code that parses the XML file is in a helper, as this logic is common to all of the factory methods. Step 3 has to be different for all the methods, so that part is found in the static classes.

  • 1
    No reason factory methods need be static. Simply make them non-static, problem solved; people have to new up the object now. Also it's easier to test code that depends on it now, you can't replace static classes with mocks as easily as non-static classes. – Jimmy Hoffa Jul 8 '13 at 16:54
  • @JimmyHoffa I see what you are saying, but at the same time, making them static makes more semantic sense as all objects odf that type are identical, there should never be a reason to have the, and all the work is done in static methods (or constructors). Is there a way to preserve the semantic meaning? – soandos Jul 8 '13 at 16:55
  • 1
    @soandos Just because a function doesn't have state doesn't mean it's not conceptually also an object. What about having the 20 classes extend Func (or Action depends on invocation) (well, technically, that's just creating your own delegates, but you get my point :) )? – Benjamin Gruenbaum Jul 8 '13 at 17:50
  • @BenjaminGruenbaum I belive it to be more "factory-like" than object, and it seems odd to new an object, invoke it, and then clean it up. – soandos Jul 8 '13 at 17:54
  • 1
    That's how 100% of the code should work. Loosely coupled: create new instantation, do something, clean up (dispose if necessary). That's what you do when opening a Textfile using i.e. streamreader. Afaik using static here is not a good idea because u remove the testability, extensibility... Code to parse files should be easily replaced with a different implementation (look up: liskov substitution). – Frederik P. Jul 9 '13 at 6:36
0

I failed to understand why one might need 20 classes here. I think it takes one class with 20+ methods.

class XMLMaker {
  public OutputClassA ParseA(File input) { ... }
  public OutputClassB ParseB(File input) { ... }
  // etc, however many classes you need to create
  //
  // reusable helper methods for the public methods above
  protected XML.Node _ParseFoo(...) {...}
  protected string _ParseBar(...) {...}
  //
  // I can imagine you might want it, too:
  public OutputClass Parse(File input) {
    var file_kind = _DetectKind(input);
    if (file_kind == KIND_A) return ParseA(input);
    // etc
  }
}

I can also imagine that you can spread ParseA, ParseB, etc into a manageable set of separate files by making XMLMaker a partial class.

XMLMaker does not need to store any state, so it can be instantiated once by a caching factory, or by a DI mechanism.

  • Each method is around 100 lines, excluding reading the XML. While I could make it all one file, it just seemed a little large. Just as an aside, these methods are creating an object, not XML – soandos Jul 8 '13 at 20:03
  • @soandos: regarding file size, did I not mention partial classes? With Java, you'd be out of luck, but C# has this nice feature. – 9000 Jul 8 '13 at 21:18
  • 1
    Thats, in my opinion, not realy a solution. Abstraction is one of the major OOP principles, where it is a must to devide your code into small, logic abstractions. Btw, partial classes are not supposed to be used for this purpose!! ( livingwithsourcecode.com/2011/12/…) (I am not aware of the TS's code, but simply stating to put everything in one class is not acceptable. – Frederik P. Jul 9 '13 at 6:44
  • 1
    Partial classes are usefull when u have your own code, and custom generated code (EF) and you do not want to lose your changes when you rerun the custom tool. Do not use partial classes to make ur classes look small. Imagine, we port the application to java, where you do not have partial classes. This will make it alot harder to port it, since in java you suddenly have large files. And people might need to refactor it to seperate classes. (which was not necessary if you had done this allready) – Frederik P. Jul 9 '13 at 6:45
  • 1
    A single class should be responsible for one thing. This approach makes one class responsible for twenty similar things. If a class can't fit on my screen it needs to be broken up into smaller classes. Also, static is not your friend when it comes to testing. Use instances of classes that have a common interface and all come from some factory class. – mortalapeman Jul 10 '13 at 12:34
2

I then wanted to have all the classes derive from one common class so that I am not referencing a seemingly unrelated class constantly, only to discover that inheritance and static do not work together well.

I was with you up to this point, but this bit sounds like a misuse of inheritance.

If you feel like your classes are too tightly bound to the helper class, concentrate on improving the public interface of the helper class.

  • Considering that the helper class contains all of the the nested components for the final object, how can I construct the obejct without doing many myTempVar.Add(helper.class...) type of things? – soandos Jul 8 '13 at 16:59
  • Could you update your question with a bit of example code? It sounds like you're talking about state there, but you specifically said there's no state, so I'm a little confused! – vaughandroid Jul 8 '13 at 17:15
  • Just updated, let me know if something is unclear – soandos Jul 8 '13 at 17:18
1
  1. Pick one of ~20 factory methods

This part is interesting and may hold the key to solving this problem. How do you 'pick' the appropriate factory method? Is there a Factory class? Without an example it's quite difficult to provide a concrete answer but the way I would envisage solving this problem is as follows:

public abstract class BaseClass
{
    //common code and maybe some abstract methods

    //This could (should?) go into it's own class.
    public static BaseClass Factory(File file)
    {
        if (something)
        {
            return Class1.Create(file) //or just call the constructor
        }
        if (somethingElse)
        {
            return Class2.Create(file)...
        }
    }
}

public class Class1 : BaseClass
{
    //private constructor so that you have to go through the Create() method
    private Class1()
    {}

    //variables/code for handling the file and populating the variables

    private void ParseFile(File file)
    {
    }

    public static Class1 Create(File file)
    {
        Class1 class1 = new Class1();
        class1.ParseFile(file);
        return class1;
    }
}

Obviously this is all pseudo-code!

I note that you say that the classes don't need to hold any state which is why they are static but something about that along with the need for inheritance doesn't quite add up. With my code you would call the Factory method on the base class (or in it's own class if you so desire) passing in each file and what you end up with is a list of BaseClasses which you pass back to the caller who presumably does something with them.

Perhaps if you could provide more detail of what you start with, what you want to end up with, and what you do in the middle we could offer some more concrete advice.

  • It picks a class based on a an enum from a refelected value in a seperate part of the application. I wish to use inheritance merely for code resuse reasons (they all need to access the Field and Group types, parse the XML the same way, etc. There is no state that is required for that – soandos Jul 8 '13 at 17:47
0

I then wanted to have all the classes derive from one common class so that I am not referencing a seemingly unrelated class constantly

That is your mental error here - you seem to believe that "inheritance" is somehow better for reuse than using a separate helper class. In fact, in most cases the opposite is the case, and since all of your classes are static, you won't get any benefits from inheritance, even if you could use that here. Your example shows exactly the kind of thinking which leads into typical wrong use of inheritance. Better give your helper class a better name so it does not seem to be so "unrelated".

  • The reason that I was thinking inheritance is because all the classes share a common goal (create a complete object, given a partial one). While an interface would have been sufficent to express that, that allows for no code reuse at all – soandos Jul 9 '13 at 15:33
  • @soandos: if your 20 classes can all be derived from a common InstanceFromXmlCreator class, and your methods have about 100 lines, I find it very likely that your methods are too long and you should refactor them into smaller, non-static methods, probably sharing some member variables. And then inheritance will probably make more sense. But honestly, without seeing the real code it is hard to give you a good answer. – Doc Brown Jul 9 '13 at 18:05
  • Ofcourse it's hard to guess what the code looks like. But you're pretty much correct. I'm pretty sure it can be rewritten in smaller "pieces" without using a static class. Static classes are good for small helpers, not for big parsing classes. Since u remove inheritance, generic parsers, inhert those generic parsers, overriding, ... by using static. If all of the classes are completely different, I guess they can stay static... But idd, approriate naming is important, and so is seperation of concerns! – Frederik P. Jul 10 '13 at 12:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.