Ineffective design on the IDomainObjectFactory?

Developer
Sep 15, 2007 at 1:22 AM
Edited Sep 15, 2007 at 1:32 AM
The responibility in the Construct method are ambigious. The method has to get all the indexes for the fields to retrieve, and the it has to retrieve the fields and contruct the object..

So in the find method for the Repository you find the following code

    while (rdr.Read())
    {
        arrray.add(DomainFactory.Construct(rdr);
    }

this is not so bad at the first sight. But when you have N object you get a cost of 2 N to create the object returned from the reader.

a much better design would have been to have a initializer who get all the indexes and was used as follows:

    if (rdr.Read())
    {
        DomainFactory.Initialize(rdr);
 
        do
        {
            arrray.add(DomainFactory.Construct(rdr);
        }
        while (rdr.Read());
    }
Developer
Sep 15, 2007 at 2:03 PM
You are absolutely correct, Benny.

I understand that they are trying to get some performance gains by not accessing the DataReader by column names, but I always wondered how effective that approach is especially when the Construct Method can be called in a loop when constructing several objects.

Like you, I didn't like the code. I tackled it a little differently by generating an enum for each table:

internal enum CustomersTable : int
{
     CustomerId = 0, Firstname = 1, Lastname,...
}

and then just use the enum in the Construct Method...

... dr[(int)CustomersTable.CustomerId] ...


I am guessing it will be up to the community to optimize that code a bit.

Great point.

Regards,

Dave

_____________________________________

David Hayden
Microsoft MVP C#
PnPGuidance
Developer
Sep 16, 2007 at 1:48 PM
Your solution is not bad, David.

The downside of your approach is that the order of the fields returned from the select/stored procedure has to be fixed. If this is a big problem must someone else tell us, but this creates a possible maintance problem.

The downside of my approach is that every IDomainObjectFactory has to have 2 methods. But since the code is factory generated it shouldnt be the biggest problem.

Regards
Benny


Developer
Sep 16, 2007 at 4:47 PM
Good point. I am going to toss your solution into my code generator to try it out.

Regards,

Dave

______________________________

David Hayden
Microsoft MVP C#
Developer
Sep 20, 2007 at 1:48 AM
Hi Benny,

Aggreed - we solved the problem by passing the following stub object:

private sealed class TableFields
{
    public readonly int Field1 = -1;
    public readonly int Field2 = -1;
}
to the following method to get initialized (via reflection) with the field indexes.

/// <summary>
/// Helper method to populate a fields class with the ordinals of all the fields in a data reader
/// </summary>
/// <param name="reader">The data reader to get ordinals from</param>
/// <param name="type">The type to create</param>
/// <returns>A new type with the ordinals populated</returns>
public object GetFields(SqlDataReader reader, Type type)
{
    Validation.RequireValue("reader", reader);
    Validation.RequireValue("type", type);
 
    // Create in instance of the object
    ConstructorInfo constructor = type.GetConstructor(System.Type.EmptyTypes);
    object target = constructor.Invoke(null);
    if (target == null)
    {
        throw new InvalidOperationException(String.Format("Unable to create {0}.", type.FullName));
    }
 
    // Get the properties from the type
    FieldInfo[] fields = type.GetFields(BindingFlags.DeclaredOnly|BindingFlags.Instance|BindingFlags.Public);
    if (fields.Length == 0)
    {
        throw new InvalidOperationException(String.Format("No publicly accessible fields have been defined in {0}.", type.FullName));
    }
 
    // Step over all the public properties in the target
    foreach (FieldInfo field in fields)
    {
        try
        {
            // Get the ordinal from the reader
            field.SetValue(target, reader.GetOrdinal(field.Name));
        }
        catch (IndexOutOfRangeException exc)
        {
            throw new InvalidOperationException(String.Format("Cannot retrieve '{0}' from the recordset.  Please check to ensure the query returned it.", exc.Message));
        }
    }
 
    return target;
}