4
\$\begingroup\$

During the test process of a device, there is a procedure that sets a device in a state in which it is ready to be tested (typically by writing default measure parameters, 3G connection parameters and any other stuff that is requested to be configurable by the devices R&D team).

The code below works but I don't like how it looks and I'd like to tidy it up.

public void TestSetupDefault()
{
    nrg.WriteFlagTest();

    NRG96.SetupConfiguration setupConfiguration = new NRG96.SetupConfiguration((int)Configuracion.GetDouble("PRIMARY_VOLTAGE", 1, ParamUnidad.SinUnidad),
        (ushort)Configuracion.GetDouble("SECONDARY_VOLTAGE", 1, ParamUnidad.SinUnidad),
        (ushort)Configuracion.GetDouble("PRIMARY_CURRENT", 1, ParamUnidad.SinUnidad),
        Configuracion.GetString("VOLTAGE_TYPES", "SIMPLES", ParamUnidad.SinUnidad),
        (NRG96.SetupConfiguration.MainDefaultScreens)Configuracion.GetDouble("MAIN_DEFAULT_SCREEN", 1, ParamUnidad.SinUnidad),
        (NRG96.SetupConfiguration.EnergyDefaultScreens)Configuracion.GetDouble("ENERGY_DEFAULT_SCREEN", 1, ParamUnidad.SinUnidad),
        Configuracion.GetString("HARMONIC_CALCULUS", "THD", ParamUnidad.SinUnidad).ToUpper() == "THD",
        (byte)Configuracion.GetDouble("BACKLIGHT_SHUTDOWN_TIME", 1, ParamUnidad.SinUnidad));

    nrg.WriteSetupConfiguration(setupConfiguration);

    NRG96.MaximumDemandConfiguration maximumDemandConfiguration = new NRG96.MaximumDemandConfiguration()
    {
        VariabletoCalculate = Configuracion.GetString("MAXIMUM_DEMAND_VARIABLE", "NO_PD", ParamUnidad.SinUnidad),
        RegisterTime = (ushort)Configuracion.GetDouble("MAXIMUM_DEMAND_REGISTER_TIME", 15)
    };

    nrg.WriteMaximumDemandConfiguration(maximumDemandConfiguration);

    NRG96.TriCalibrationFactors compoundFactors = new NRG96.TriCalibrationFactors((ushort)Configuracion.GetDouble("COMPOUND_VOLTAGE_CALIBRATION_FACTOR", 8790, ParamUnidad.SinUnidad));

    nrg.WriteCompoundvoltageFactors(compoundFactors);


}

By the way I have created the objects in 2 ways, using a constructor first, and setting properties (object initializer). Which one is more correct?

To conclude, the question I am asking is:

How can I tidy up this code to make it more readable?

I feel like developing separate functions for each structure is like hiding this code with regions and I don't quite like that.

Keep in mind this is not about performance but about readability, I feel like there is no more code needed since what the struct does with the data or how I send it is not what I am asking to be reviewed, just how to properly display the code since that is a field I have much to work on.

\$\endgroup\$
2
  • \$\begingroup\$ What does Configuracion.GetDouble()et al do? Can it be simplified to make this code less verbose? \$\endgroup\$ Apr 20, 2016 at 14:04
  • \$\begingroup\$ It is an interaction with a database to retrieve a value from a given key (the first parameter), in case the value is not found a default value is returned and queued to be stored in the database (second parameter), the third (optional) parameter indicates the unit of the field for the database, so as to being able to create reports about the device \$\endgroup\$ Apr 20, 2016 at 14:07

2 Answers 2

2
\$\begingroup\$

There are several possible steps you can take to increase readability to your code, depending on how much control of the code you have and how much implementation details you which to hide.

Separate default values from database loading logic

By initializing setupConfiguration with default values by named properties and moving the database loading logic into a LoadFromDatabase() method you increase readability of the defaults:

var setupConfiguration = Configuracion.LoadFromDatabase(new NRG96.SetupConfiguration
{
    PRIMARY_VOLTAGE = 1,
    SECONDARY_VOLTAGE = 1,
    PRIMARY_CURRENT = 1,
    VOLTAGE_TYPES = "SIMPLES",
    MAIN_DEFAULT_SCREEN = SetupConfiguration.MainDefaultScreens.One,
    ENERGY_DEFAULT_SCREEN = SetupConfiguration.EnergyDefaultScreens.One,
    HARMONIC_CALCULUS = "THD",
    BACKLIGHT_SHUTDOWN_TIME = 1
});

I'm assuming SetupConfiguration can be defined by something like:

public class SetupConfiguration
{
    public int PRIMARY_VOLTAGE { get; set; }
    public ushort SECONDARY_VOLTAGE { get; set; }
    public ushort PRIMARY_CURRENT { get; set; }
    public string VOLTAGE_TYPES { get; set; }
    public MainDefaultScreens MAIN_DEFAULT_SCREEN { get; set; }
    public EnergyDefaultScreens ENERGY_DEFAULT_SCREEN { get; set; }

    public string HARMONIC_CALCULUS { get; set; }
    public byte BACKLIGHT_SHUTDOWN_TIME { get; set; }

    // Doesn't get saved to database because no public setter
    public bool HARMONIC_CALCULUS_IS_THD { get { return (HARMONIC_CALCULUS ?? "").ToUpper() == "THD"; } }



    public enum MainDefaultScreens
    {
        One = 1,
        Two
    }
    public enum EnergyDefaultScreens
    {
        One = 1,
        Second
    }
}

LoadFromDatabase() could then be implemented by:

public static SetupConfiguration LoadFromDatabase(SetupConfiguration defaultValues)
{
    return new SetupConfiguration
    {
        PRIMARY_VOLTAGE = (int)GetDouble("PRIMARY_VOLTAGE", defaultValues.PRIMARY_VOLTAGE, ParamUnidad.SinUnidad),
        SECONDARY_VOLTAGE = (ushort)GetDouble("SECONDARY_VOLTAGE", defaultValues.SECONDARY_VOLTAGE, ParamUnidad.SinUnidad),
        PRIMARY_CURRENT = (ushort)GetDouble("PRIMARY_CURRENT", defaultValues.PRIMARY_CURRENT, ParamUnidad.SinUnidad),
        VOLTAGE_TYPES = GetString("VOLTAGE_TYPES", defaultValues.VOLTAGE_TYPES, ParamUnidad.SinUnidad),
        MAIN_DEFAULT_SCREEN = (SetupConfiguration.MainDefaultScreens)GetDouble("MAIN_DEFAULT_SCREEN", (int)defaultValues.MAIN_DEFAULT_SCREEN, ParamUnidad.SinUnidad),
        ENERGY_DEFAULT_SCREEN = (SetupConfiguration.EnergyDefaultScreens)GetDouble("ENERGY_DEFAULT_SCREEN ", (int)defaultValues.ENERGY_DEFAULT_SCREEN, ParamUnidad.SinUnidad),
        HARMONIC_CALCULUS = GetString("HARMONIC_CALCULUS", defaultValues.HARMONIC_CALCULUS, ParamUnidad.SinUnidad),
        BACKLIGHT_SHUTDOWN_TIME = (byte)GetDouble("BACKLIGHT_SHUTDOWN_TIME", defaultValues.BACKLIGHT_SHUTDOWN_TIME, ParamUnidad.SinUnidad)
    };
}

Cleaning up database loading logic with helper methods

LoadFromDatabase() could be furter cleaned up by creating a couple of helper methods to do the actual loading from database:

static int Get(string name, int defaultValue) { return (int)GetDouble(name, defaultValue, ParamUnidad.SinUnidad); }
static byte Get(string name, byte defaultValue) { return (byte)GetDouble(name, defaultValue, ParamUnidad.SinUnidad); }
static ushort Get(string name, ushort defaultValue) { return (ushort)GetDouble(name, defaultValue, ParamUnidad.SinUnidad); }
static string Get(string name, string defaultValue) { return GetString(name, defaultValue, ParamUnidad.SinUnidad); }

public static SetupConfiguration LoadFromDatabase(SetupConfiguration defaultValues)
{
    return new SetupConfiguration
    {
        PRIMARY_VOLTAGE = Get("PRIMARY_VOLTAGE", defaultValues.PRIMARY_VOLTAGE),
        SECONDARY_VOLTAGE = Get("SECONDARY_VOLTAGE", defaultValues.SECONDARY_VOLTAGE),
        PRIMARY_CURRENT = Get("PRIMARY_CURRENT", defaultValues.PRIMARY_CURRENT),
        VOLTAGE_TYPES = Get("VOLTAGE_TYPES", defaultValues.VOLTAGE_TYPES),
        MAIN_DEFAULT_SCREEN = (SetupConfiguration.MainDefaultScreens)Get("MAIN_DEFAULT_SCREEN", (int)defaultValues.MAIN_DEFAULT_SCREEN),
        ENERGY_DEFAULT_SCREEN = (SetupConfiguration.EnergyDefaultScreens)Get("ENERGY_DEFAULT_SCREEN ", (int)defaultValues.ENERGY_DEFAULT_SCREEN),
        HARMONIC_CALCULUS = Get("HARMONIC_CALCULUS", defaultValues.HARMONIC_CALCULUS),
        BACKLIGHT_SHUTDOWN_TIME = Get("BACKLIGHT_SHUTDOWN_TIME", defaultValues.BACKLIGHT_SHUTDOWN_TIME)
    };
}

Automating database loading logic by reflecting on the configuration objects

Finally, if you want true magic and can make sure the configuration classes properties are named as in the database, you can automate LoadFromDatabase() with something like:

public static T LoadFromDatabase<T>(T defaultValues) where T : class, new()
{
    var result = new T();

    var getters = typeof(T).GetProperties(System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance);
    foreach (var property in getters)
    {
        if (property.GetMethod != null && property.GetMethod.IsPublic
            && property.SetMethod != null && property.SetMethod.IsPublic)
        {
            var defaultValue = property.GetValue(defaultValues);
            if (property.PropertyType.Equals(typeof(int)))
            {
                var value = (int)GetDouble(property.Name, (int)defaultValue, ParamUnidad.SinUnidad);
                property.SetValue(result, value);
            }
            else if (property.PropertyType.Equals(typeof(byte)))
            {
                var value = (byte)GetDouble(property.Name, (byte)defaultValue, ParamUnidad.SinUnidad);
                property.SetValue(result, value);
            }
            else if (property.PropertyType.Equals(typeof(ushort)))
            {
                var value = (ushort)GetDouble(property.Name, (ushort)defaultValue, ParamUnidad.SinUnidad);
                property.SetValue(result, value);
            }
            else if (property.PropertyType.Equals(typeof(string)))
            {
                var value = GetString(property.Name, (string)defaultValue, ParamUnidad.SinUnidad);
                property.SetValue(result, value);
            }
            else if (property.PropertyType.BaseType.Name == "Enum")
            {
                var value = (int)GetDouble(property.Name, (int)defaultValue, ParamUnidad.SinUnidad);
                property.SetValue(result, value);
            }
        }
    }

    return result;
} 

... and call the method with:

var setupConfiguration = NRG96.Configuracion.LoadFromDatabase<NRG96.SetupConfiguration>(new NRG96.SetupConfiguration
{
    PRIMARY_VOLTAGE = 1,
    SECONDARY_VOLTAGE = 1,
    PRIMARY_CURRENT = 1,
    VOLTAGE_TYPES = "SIMPLES",
    MAIN_DEFAULT_SCREEN = SetupConfiguration.MainDefaultScreens.One,
    ENERGY_DEFAULT_SCREEN = SetupConfiguration.EnergyDefaultScreens.One,
    HARMONIC_CALCULUS = "THD",
    BACKLIGHT_SHUTDOWN_TIME = 1
});

Loading all different configuration objects from the database could then be achieved without changing LoadFromDatabase<>().

\$\endgroup\$
7
  • 1
    \$\begingroup\$ There is an IsEnum property on Type. You can use it instead of type.BaseType.Name == "Enum". \$\endgroup\$
    – Xiaoy312
    Apr 20, 2016 at 20:42
  • \$\begingroup\$ @Xiaoy312 Good point. I kind of ran out of time. The method should definately be improved like that. \$\endgroup\$ Apr 21, 2016 at 5:30
  • \$\begingroup\$ Another possible generalization for LoadFromDatabase<T>() would be to scan the entities for a custom attribute and get database name and type from that. Then the properties wouldn't need to be hard-wired to the database any more. \$\endgroup\$ Apr 21, 2016 at 5:33
  • \$\begingroup\$ This is kind of a good approach to what I desired, a question though, my job coleague tells me it is not correct to develop a function for something I am going to call only once (namely the retrieval of default configuration of a device for testing purposes), and that it is not needed as it is equivalent to working with Regions. Is that right? \$\endgroup\$ Apr 21, 2016 at 8:39
  • 1
    \$\begingroup\$ @Oscar I confess to the church of self-documenting code. As such I don't agree at all with your fellow collegues. I think regions are great to hide code blocks (methods) you don't need to look at every day, but believe blocks of code shall be shortt, self-documenting and reusable - even if you don't reuse them today. That is good for a SOLID approach. \$\endgroup\$ Apr 21, 2016 at 14:01
0
\$\begingroup\$

I would break out the parameter fetch from the setupConfiguration constructor.

Also, in MaximumDemandConfiguration, you are declaring two variable that you never reference. While I have left them in, you could remove the variable names or declare the parameters just like all the others and then use the variable names in the constructor.

Finally, you can use a namespace alias to shorten the amount of text in the namespace: using SC = NRG96.SetupConfiguration. You can do this for other namespaces as well.

public void TestSetupDefault()
{
    nrg.WriteFlagTest();

    int pPrimaryVoltage = (int)Configuracion.GetDouble("PRIMARY_VOLTAGE", 1, ParamUnidad.SinUnidad);
    ushort pSecondaryVoltage = (ushort)Configuracion.GetDouble("SECONDARY_VOLTAGE", 1, ParamUnidad.SinUnidad);
    ushort pPrimaryCurrent = (ushort)Configuracion.GetDouble("PRIMARY_CURRENT", 1, ParamUnidad.SinUnidad);
    string pVoltageType = Configuracion.GetString("VOLTAGE_TYPES", "SIMPLES", ParamUnidad.SinUnidad);
    SC.MainDefaultScreens pMainDefaultScreen = (NRG96.SetupConfiguration.MainDefaultScreens)Configuracion.GetDouble("MAIN_DEFAULT_SCREEN", 1, ParamUnidad.SinUnidad);
    SC.EnergyDefaultScreens pEnergyDefaultScreen = (NRG96.SetupConfiguration.EnergyDefaultScreens)Configuracion.GetDouble("ENERGY_DEFAULT_SCREEN", 1, ParamUnidad.SinUnidad);
    bool pIsThdHarmonic = (Configuracion.GetString("HARMONIC_CALCULUS", "THD", ParamUnidad.SinUnidad).ToUpper() == "THD");
    byte pBacklightShutdownTime = (byte)Configuracion.GetDouble("BACKLIGHT_SHUTDOWN_TIME", 1, ParamUnidad.SinUnidad)

    NRG96.SetupConfiguration setupConfiguration = new NRG96.SetupConfiguration(
         pPrimaryVoltage,
         pSecondaryVoltage,
         pPrimaryCurrent,
         pVoltageType,
         pMainDefaultScreen,
         pEnergyDefaultScreen,
         pIsThdHarmonic,
         pBacklightShutdownTime);

    nrg.WriteSetupConfiguration(setupConfiguration);

    NRG96.MaximumDemandConfiguration maximumDemandConfiguration = new NRG96.MaximumDemandConfiguration()
    {
        VariabletoCalculate = Configuracion.GetString("MAXIMUM_DEMAND_VARIABLE", "NO_PD", ParamUnidad.SinUnidad),
        RegisterTime = (ushort)Configuracion.GetDouble("MAXIMUM_DEMAND_REGISTER_TIME", 15)
    };

    nrg.WriteMaximumDemandConfiguration(maximumDemandConfiguration);

    NRG96.TriCalibrationFactors compoundFactors = new NRG96.TriCalibrationFactors((ushort)Configuracion.GetDouble("COMPOUND_VOLTAGE_CALIBRATION_FACTOR", 8790, ParamUnidad.SinUnidad));

    nrg.WriteCompoundvoltageFactors(compoundFactors);
}
\$\endgroup\$

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.