1
\$\begingroup\$

I'm building a method that tries to detect whether given information about an object's property to retrieve, a constraint operator and some data to compare it against, whether the result is true or false.

So I'm given is an object containing the type of object to read (File, Folder, RegistryKey, RegistryValue, MSI), the information required to retrieve that particular object type (for example a path, or an MSI product code) and the name of the property to read from that object as well as what data type that property should be. There are only four data types that can be returned: string, int, DateTime and Version.

I'm also given a constraint operator which can be (among a couple others) Equals, Contains, StartsWith, EndsWith, Between, GreaterThan, LowerThan, OneOf. Some will only accept strings (Contains, StartsWith, EndsWith) others will accept the other three object types but not strings (Between, GreaterThan, LowerThan) and some will accept all object types (Equals, OneOf). Depending on the constraint operator itself, I may either have a single value to compare against (Equals, StartsWith, EndsWith, GreaterThan, etc), two values (Between) or an array of values (OneOf). There are also And and Or constraint operators that basically just contain an array of other constraint operator expressions.

Lastly I also have what to compare against, which is just a value as a string and the data type that should be (so I need to cast it to the correct data type).

My initial idea was to try to break things up and have one method responsible for a single thing. So one method to retrieve the object's property given the data that I have, one method responsible for retrieving the appropriate object to compare against (and cast it to the correct data type) and one method responsible for executing the constraint operator's logic and return a definitive bool value.

My first problem is that because my method to return the object's property can actually return 4 different data types... I had to make it return object. I could make it a generic method, but then that would imply that the calling would have to know the data type of the property being returned and that feels like it's outside of the scope of the calling method, it should just go "here's some data that you need to analyze to return an object's property back to me. I don't care how, I don't care what it is, just do it".

The second problem is pretty much the same but for the data I want to compare against.

The third problem is with the constraint operators, I thought of a way of creating an interface that fits all of them and came up with a (rather dodgy) interface implementation as follows:

public interface IConstraintOperator
{
    public bool Check<T>(T valueA, params T[] valueB)
}

This allows me never care for the concrete type implementing the interface and just deal with the interface, but also means I never have to care about what T is or what I'm passing to it (be it a single value, or two values or an array, etc).

There are of course quite a few disadvantages to this:

  • For each class implementing IConstraintOperator I need to check that T is a supported type and potentially handle it differently internally and throw if typeof(T) is not supported.

  • I receive params T[] as my second parameter, which means that for constraint operator that only care about one value I ignore anything other than the first value. It's not a bug at all, but it's also not pretty code when I can do Equals("Hello", "H", "e", "l", "l", "o") and only the first element of the array is used for the comparison.

How can I improve these things?

Current working code below:

private bool DetectExpression(Expression expression, IEnumerable<SimpleSetting> settings)
{
    var op = Enum.Parse<ConstraintOperator>(expression.Operator.OperatorName, true);

    if (op == ConstraintOperator.AND)
        return expression.Operands.Select(x => DetectExpression((Expression) x, settings)).All(x => x);

    if (op == ConstraintOperator.OR)
        return expression.Operands.Select(x => DetectExpression((Expression) x, settings)).Any(x => x);

    var settingReference = (SettingReference) expression.Operands.FirstOrDefault(x => x is SettingReference);
    var constantValue = (ConstantValue) expression.Operands.FirstOrDefault(x => x is ConstantValue);
    var constantValueList = (ConstantValueList) expression.Operands.FirstOrDefault(x => x is ConstantValueList);

    object constantValueValue = constantValue?.ValueType.Name switch
    {
        nameof(Int64) => long.Parse(constantValue.Value),
        nameof(Version) => Version.Parse(constantValue.Value),
        nameof(DateTime) => DateTime.Parse(constantValue.Value),
        nameof(String) => constantValue.Value,
        null => null,
        _ => throw new NotSupportedException()
    };

    var constantValueListValue = constantValueList?.ListValueType.Name switch
    {
        "Int64Array" => constantValueList.Values.Select(long.Parse).Cast<object>(),
        "VersionArray" => constantValueList.Values.Select(Version.Parse),
        "DateTimeArray" => constantValueList.Values.Select(DateTime.Parse).Cast<object>(),
        "StringArray" => constantValueList.Values,
        null => null,
        _ => throw new NotSupportedException()
    };

    var objectProperty = GetObjectProperty(settingReference, settings);

    return GetConstraintOperator(op).Check(objectProperty, constantValueValue ?? constantValueListValue);
}

private static object GetObjectProperty(SettingReferenceBase settingReference, IEnumerable<SimpleSetting> settings)
{
    var obj = GetObject(settingReference, settings);
    
    if (settingReference.MethodType == ConfigurationItemSettingMethodType.Count)
        return obj != null ? 1 : 0;

    if (obj == null)
        return null;

    return settingReference.PropertyPath switch
    {
        // Registry key
        "RegistryKeyExists" => true,
        // Registry value
        "RegistryValueExists" => true,
        "" when settingReference.SettingSourceType == ConfigurationItemSettingSourceType.Registry => obj,
        // File
        "Size" => ((FileObject) obj).Size,
        "Version" when settingReference.SettingSourceType == ConfigurationItemSettingSourceType.File => ((FileObject) obj).Version,
        "DateModified" when settingReference.SettingSourceType == ConfigurationItemSettingSourceType.File => ((FileObject) obj).DateModified,
        "DateCreated" when settingReference.SettingSourceType == ConfigurationItemSettingSourceType.File => ((FileObject) obj).DateCreated,
        // Folder
        "DateModified" when settingReference.SettingSourceType == ConfigurationItemSettingSourceType.Folder => ((FolderObject) obj).DateModified,
        "DateCreated" when settingReference.SettingSourceType == ConfigurationItemSettingSourceType.Folder => ((FolderObject) obj).DateCreated,
        // Not supported
        _ => throw new NotSupportedException()
    };
}

private static object GetObject(SettingReferenceBase settingReference, IEnumerable<SimpleSetting> settings)
{
    var setting = settings.First(x => x.LogicalName == settingReference.SettingLogicalName);

    // ReSharper disable once SwitchStatementHandlesSomeKnownEnumValuesWithDefault
    switch (settingReference.SettingSourceType)
    {
        case ConfigurationItemSettingSourceType.File:
            return FileObject.Get(setting.Location, ((FileOrFolder) setting).Is64Bit);
        case ConfigurationItemSettingSourceType.Folder:
            return FolderObject.Get(setting.Location, ((FileOrFolder) setting).Is64Bit);
        case ConfigurationItemSettingSourceType.RegistryKey:
            var registryKey = (RegistryKey) setting;
            return RegistryKeyObject.Get(registryKey.RootKey, registryKey.Key, registryKey.Is64Bit);
        case ConfigurationItemSettingSourceType.Registry:
            var registrySetting = (RegistrySetting) setting;
            var dataType = registrySetting.SettingDataType.Name;

            return dataType switch
            {
                nameof(String) => RegistryValueObject<string>.Get(registrySetting.RootKey, registrySetting.Key,
                    registrySetting.ValueName, registrySetting.Is64Bit)?.Value,
                nameof(Version) => RegistryValueObject<Version>.Get(registrySetting.RootKey, registrySetting.Key,
                    registrySetting.ValueName, registrySetting.Is64Bit)?.Value,
                nameof(Int64) => RegistryValueObject<long>.Get(registrySetting.RootKey, registrySetting.Key,
                    registrySetting.ValueName, registrySetting.Is64Bit)?.Value,
                _ => null
            };
        case ConfigurationItemSettingSourceType.MSI:
            var msiSetting = (MSISettingInstance) setting;
            return WindowsInstallerObject.Get(msiSetting.ProductCode);
        default:
            throw new NotSupportedException();
    }
}

public IConstraintOperator GetConstraintOperator(ConstraintOperator constraintOperator)
{
    return _constraintOperators.First(x => Enum.Parse<ConstraintOperator>(x.GetType().Name, true) == constraintOperator);
}

I actually think that the constraint operators should be extension methods/static methods with one method per each supported data type and started writing that class already, but I just can't see how to integrate it into what I already have simply because it means my DetectExpression method now needs to know the concrete data type being returned by GetObjectProperty.

Any help with refactoring this and any other issues someone might see with it?

\$\endgroup\$

Your Answer

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

Browse other questions tagged or ask your own question.