0

I'm a bit trapped in my own design. I have to extend it far beyond initial expectations.

I'll first show how things are now and what behaviours I want to add but don't know how.

Currently, the following code works fine. Some things may be discussed (like the builders class, which exists for the sake of Single Responsibility Principle but are maybe not the smartest way to do it, or the weird return type covariance workaround). Sorry for the incoming wall of code, i tried as much as i could to show you the minimal code to understand the problem.

public class QueryManager
{
    // filled with different instances of Query in constructor depending on parameters
    public List<Query> QueryList {get; set;} 

    public List<IRecords> RecordsList { get; set; }

    public virtual void Queries()
    {
        foreach (var query in QueryList)
        {       
            query.ExecutePreparedQuery();
            var listResult = query.GetRecordsFromResults();

            // i'm aware of the possibility of NullArgumentException here, 
            // i just simplify the actual thing
            RecordsList.AddRange(listResult); 
        }
    }
}


public abstract class Query
{
    public FrameworkTypeHoldingResults Result { get; protected set; }

    // Workaround for C# return type covariance.
    protected virtual IRecordBuilder RecordBuilder
    {
       get { return GetRecordBuilder(); }
    }
    protected abstract IRecordBuilder GetRecordBuilder();



    protected WebObject Web {get; set;}

    public virtual void ExecutePreparedQuery()
    {
        var query = new FrameworkTypeQuery { ... }
        Result = FrameworkType.GetItems(query)
    }

    protected virtual string DateFormatting() { ... }

    public abstract IList<IRecords> GetRecordsFromResults();
    public abstract string QueryDescription();
}


public class RealQuery : Query
{
    protected override IRecordBuilder GetRecordBuilder()
    {
        return RecordBuilder;
    }
    protected new ConcreteRecordBuilderA RecordBuilder { get; set; }

    public override string QueryDescription() => "I'm a RealQuery";

    public override IList<IRecords> GetRecordsFromResults()
    {
        var recordsList = new List<IRecords>();

            if (Result != null && Result.Count != 0)
            {
                RecordBuilder.Value = Result.Count;
                RecordBuilder.Querytype = QueryDescription();
                RecordBuilder.Dateformatted = DateFormatting();
                RecordBuilder.WebTitle = Web.Title;

                recordsList.Add(RecordBuilder.BuildRecord());

                return recordsList;
            }

            RecordBuilder.Value = 0;
            RecordBuilder.Querytype = QueryDescription();
            RecordBuilder.Dateformatted = DateFormatting();
            RecordBuilder.WebTitle = Web.Title;

            recordsList.Add(RecordBuilder.BuildRecord());
            return recordsList;
    }
}

public interface IRecords{}

public interface IRecordBuilder
{
    IRecords BuildRecord();
}

public class RecordsA 
{
    public RecordsA(int? Value, string QueryType, string Type, string Month) {...}
    public int? Value {get; private set;}
    public string QueryType {get; private set;}
    public string Web {get; private set;}

    public string Month {get; private set;}
}

public class RecordsB 
{
    public RecordsB(int? Value, string QueryType, string Type, string Month) {...}
    public int? Value {get; private set;}
    public string QueryType {get; private set;}
    public string Web {get; private set;}

    public string User {get; private set;}
}

public class RecordABuilder
{
    public int? Value {get; set;}
    public string QueryType {get; set;}
    public string Web {get; set;}

    public string Month {get; set;}

    public IRecord BuildRecord() => new RecordA(Value, QueryType, Type, Month)
}

public class RecordBBuilder
{
   // pretty much the same logic
}

We can notice that i'm just using metadatas of my query result and not the query results themselves.

Now here is my actual problem. I have a need for a new kind of Query. The problem with this query, unlike the previous one, is that i don't know the nature of the IRecords i'll get. I'll show you where it query its data.

Web  | QueryType | Value | Month       | User
foo  | B type    |    42 | April 2020  |
bar  | A type    |   777 |             | "John Doe"

As you see here, when Month is filled, User isn't (ans the opposite). If the query get the first line, I will need a RecordA instance, but if the query get the second, I will need a RecordB instance. The query, this time, directly uses the query results, and the class doesn't know what it will get. The problem with if-condition is that if more columns and more IRecords are added, if will great a giant if-condition tree.

So far, this is the GetRecordsFromResults implementation I have, but it only works for one implementation of IRecords, not both :

public override IList<IRecords> GetRecordsFromResults()
{
    var recordsList = new List<IRecords>();

    if (Result != null && Result.Count != 0)
    {
        for (var index = 0; index < Result.Count; index++)
        {
            var it = Result[index];

            RecordBuilder.Value = int.Parse(it["Value"].ToString());
            RecordBuilder.Querytype = it["QueryType"] as string;

            // But maybe i want the User column instead...
            RecordBuilder.Month= it["Month"] as string; 
            RecordBuilder.WebTitle = it["Web"] as string;

            recordsList.Add(RecordBuilder.BuildRecord());

            // would a  RecordsBBuilder in some cases
            RecordBuilder = new RecordsABuilder(); 
        }
        return recordsList;
    }

    RecordBuilder.Value = 0;
    RecordBuilder.Querytype = QueryDescription();

    // But maybe i want the User column instead...
    RecordBuilder.Month= DateFormatting(); 
    RecordBuilder.Web = Web.Title;

    recordsList.Add(RecordBuilder.BuildRecord());

    // would a  RecordsBBuilder in some cases
    RecordBuilder = new RecordsPerMonthBuilder(); 

    return recordsList;
}

I'm a bit stuck here, do you have any idea how i could implement this new Query concrete type ?

EDIT : I didn't make clear that the relationship between Query implementation and IRecords isn't one for one. RecordA is for a family of Query implementations (identified by an abstract subclass between Query and its implementations) and RecordB for direct (no subclass in between) implementations. There may also be RecordC in the future. Each Query implementation got its self describing method that is used to fill the QueryType property in IRecords implementations.

  • Is there a reason none of your methods there have any parameters? If your IRecordBuilder interface was instead Func<int?, string, string, FrameworkTypeHoldingResults, IRecords> you'd have a lot less ceremony getting in the way. As is using IRecordBuilder = Func... – Caleth May 19 '17 at 10:32
  • RecordA and RecordB don't have the same properties, future potential RecordC may have more too for exemple. Also, i just figured out .NET version i'm using (no choice here) only allows Func<T1, T2, T3, T4, TResults>. If i have one more parameter in a future RecordC, i would have to rewrite everything. Also the solution proposed by John Wu below, with func<>, suppose the RecordBuilder knows every single possible Query implementation. – Ythio Csi May 19 '17 at 11:10
  • Methods in Query have no parameters because they use mostly local fields/properties initialised in constructor. I could remove Record properties and just pass in BuildRecord method however. – Ythio Csi May 19 '17 at 11:14
  • What I mean is that an IRecordBuilder is a function from Dictionary<string, string> + some shared values to IRecord. You can then use @JohnWu's example to compose multiple IRecordBuilders into a CompositeRecordBuilder, that is itself an IRecordBuilder – Caleth May 19 '17 at 11:21
  • I actually implemented @JohnWu's solution and will test it. It solve many thing and as a bonus allows me to remove the workaround for C# return type covariance that i didn't really liked. However it bothers me that a class tasked to instantiate other object has to know every single real implementation of an abstract class (one of those implementation being the calling class). Maybe it is just me who got a wrong predjudice. – Ythio Csi May 19 '17 at 11:34
2

I think instead of a RecordABuilder you need a RecordBuilder with code a bit like this:

public IRecord BuildRecord() => QueryType == "A" ? new RecordA(Value, QueryType, Type, Month) 
                                                 : new RecordB(Value, QueryType, Type, Month);

If you plan for more than two record types, the ternary operator won't do. Lambdas to our rescue:

public class RecordBuilder
{
    public int? Value {get; set;}
    public string QueryType {get; set;}
    public string Web {get; set;}
    public string Month {get; set;}

    private readonly Dictionary<
        string, 
        Func<int, string, string, string, IRecord>> activators = 

        new Dictionary<
        string, 
        Func<int, string, string, string, IRecord>>();

    public RecordBuilder()
    {
        activators.Add("A",(v,q,t,m) => new RecordA(v, q, t, m));
        activators.Add("B",(v,q,t,m) => new RecordB(v, q, t, m));
        activators.Add("C",(v,q,t,m) => new RecordC(v, q, t, m));
    }

    public IRecord BuildRecord() => activators[QueryType](
        Value, 
        QueryType, 
        Type, 
        Month);
}

Now the question arises-- what if the constructors for RecordA and RecordB are different? We can accommodate that by populating the constructor arguments from closures instead of arguments to the lambda, like so:

public class RecordBuilder
{
    public int? Value {get; set;}
    public string QueryType {get; set;}
    public string Web {get; set;}
    public string Month {get; set;}

    private readonly Dictionary<string, Func<IRecord>> activators = 
                 new Dictionary<string, Func<IRecord>>();

    public RecordBuilder()
    {
        activators.Add("A",() => new RecordA(Value, QueryType, Web, Month));
        activators.Add("B",() => new RecordB(Value, QueryType));
        activators.Add("C",() => new RecordC(Value, QueryType, Month));
    }

    public IRecord BuildRecord() => activators[QueryType]();
}

In the lambda expressions above, the call to new will draw its variables from the fields in class scope. The variables will be enclosed in a closure so the lambdas will always get the current value when executed.

And of course you could always go to using a case/switch. While this seems a little inelegant, it is a perfectly reasonable way to go in a factory.

public class RecordBuilder
{
    public int? Value {get; set;}
    public string QueryType {get; set;}
    public string Web {get; set;}
    public string Month {get; set;}

    public IRecord BuildRecord()
    {
        switch (QueryType)
        {
            case "A": return new RecordA(Value, QueryType, Web, Month);
            case "B": return new RecordB(Value, QueryType);
            case "C": return new RecordC(Value, QueryType, Month);
            default: throw new InvalidOperationException();
        }
    }
}

Note that the above requires the factory to know how to map a string containing a query type to a concrete instance. This is common and customary for a factory. But, if you really are married to the idea that the factory shouldn't know ahead of time what types are available or how to build them, you could do something with reflection to get an inventory of all types that implement IRecord and use an attribute to determine which class goes with which code. To deal with the problem of a variable constructor, you could just pass the builder itself as the constructor argument and let the concrete class pull whatever it needs.

This is a little more complicated, but here goes:

class RecordTypeAttribute : System.Attribute
{
    public string QueryType { get; private set; }
    public RecordTypeAttribute(string queryType)
    {
        QueryType = queryType;
    }
}
interface IRecord
{
    string QueryType { get; }
}

[RecordType("A")]
class RecordA : IRecord
{
    public string QueryType { get; private set; }

    public RecordA(RecordBuilder dataSource)
    {
        QueryType = dataSource.QueryType;
    }
}

[RecordType("B")]
class RecordB : IRecord
{
    public string QueryType { get; private set; }
    public int? Value { get; set; }

    public RecordB(RecordBuilder dataSource)
    {
        QueryType = dataSource.QueryType;
        Value = dataSource.Value;
    }
}

class RecordBuilder
{
    public string QueryType { get; set; }

    private Dictionary<string,Type> GetAvailableRecordTypes()
    {
        var list = new Dictionary<string, Type>();
        foreach (Assembly asm in AppDomain.CurrentDomain.GetAssemblies())
        {
            foreach (Type t in asm.GetExportedTypes())
            {
                if (typeof(IRecord).IsAssignableFrom(t))
                {
                    RecordTypeAttribute a = System.Attribute.GetCustomAttributes(t).OfType<RecordTypeAttribute>().FirstOrDefault();
                    if (a != null)
                    {
                        list.Add(a.QueryType, t);
                    }
                }
            }
        }
        return list;
    }
    public IRecord BuildRecord()
    {
        var typesAvailable = GetAvailableRecordTypes();
        var type = typesAvailable[QueryType];
        return System.Activator.CreateInstance(type, new [] { this }) as IRecord;
    }
}

The above is overkill in most cases, although it is a neat pattern when you want a pluggable architecture.

Note: This class should probably be named RecordFactory instead of RecordBuilder as it does not follow the builder pattern and is pretty much a factory. Once you realize the misnaming, the question of where to implement the logic that decides which record to create is immediately answered.

  • But RecordB and RecordA don't have the exact same properties. I can't overload BuildRecord since Month and User are both string types. Due to .NET version, it seems i'm limited to Func<T1, T2, T3, T4, TResult> so a future RecordC with more arguments wouldn't fit and i would have to rewrite everything (i just figured it out) Also, the knowledge of existing Query implementations (when you do "A", "B", "C", etc...) by the RecordBuilder bothers me a bit. Is it really his role to know this ? – Ythio Csi May 19 '17 at 11:19
  • Closures to the rescue. Edited. And yes, it is exactly the role of a factory to map QueryType values to their corresponding record type. It has to happen somewhere, and the factory pattern is intended for this purpose. – John Wu May 19 '17 at 15:59
  • Forgot my previous comment (i deleted it), actually i understood. Thank you so much !! :D – Ythio Csi May 19 '17 at 16:21
  • Just for fun, I added some code that allows you to let record type classes define their own query types via attributes. The factory can find the appropriate class using reflection. I think it adds unnecessary complexity but it is an option. – John Wu May 19 '17 at 16:37
  • Oh that's very neat. I also think it adds unnecessary complexity, but it is interesting for me to learn how to do this !! To avoid specifying "A" everywhere in the code, i made a little class with a field static readonly MyClass QUERY_A = new MyClass("A"), private readonly string _queryType and constructor private MyClass(string name) { _queryType = name } (+ override string ToString() => _queryType). I use it kind of like an enum so if "A" is renamed "AA" i can change this only once. – Ythio Csi May 22 '17 at 8:34

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.