Mapping various input values to a data accumulation class

Question

This function takes inputs from multiple different controls on a form, and maps them to values in a data accumulation class. This code was written specifically to keep logic, data, and display separated. I ended up at the time using reflection to take the values from the controls and mapping them to variables with the same names in the accumulation class. The reasoning behind doing it this way was I didn't want a bunch of:

variable = control.value

Calls being repeated throughout this section of code, and should it end up that I needed additional controls on that form, I wanted to be able to just add the appropriate variable to the accumulation class and let the program handle adding that information to the accumulator dynamically.

I'm posting here to see what if any better ways to do this exist. (I'm sure there must be one, but this was the method I found that works.)

What I've written works well, and does it's job, but feels like very very bad code.

This is the mapping method:

/// <summary>
    /// Updates the random accumulator.  One method to rule *ALL* The random controls!
    /// </summary>
    /// <param name="sender"></param>
    /// <param name="e"></param>
    private void UpdateRandomAccumulator ( object sender, EventArgs e ) {
        //Dear Future Me: You are a genius.  You made this work.  Despite overwhelming odds, you are victorious!
        //Be proud future me, when you look upon this method in total confusion, since you probably forgot 
        //*exactly how* it works 10 minutes after closing this class.

        Control control = sender as Control;
        if ( control is TrackBar ) {
            TrackBar tbar = control as TrackBar;
            string controlName = control.Name.Remove ( 0, 9 );
            //okay limiting our MinBads/MaxTourists here.
            if ( controlName.Equals ( "MinNumberOfBadasses" ) || controlName.Equals ( "MaxNumberOfTourists" ) ) {
                if ( tbar.Value >= randAccum.NumberToCreate ) {
                    tbar.Value = randAccum.NumberToCreate;
                }
            }
            foreach ( var prop in randAccum.GetType( ).GetProperties( ) ) {
                if ( controlName == prop.Name ) {
                    if ( prop.PropertyType == typeof(float) ) {
                        prop.SetValue ( randAccum, (float) tbar.Value / 100 );
                    }
                    else if ( prop.PropertyType == typeof(int) ) {
                        prop.SetValue ( randAccum, tbar.Value );
                    }
                    else {
                        Logger.LogEvent ( prop.PropertyType.ToString ( ) );
                    }
                }
            }
            tbar_rnd_MaxNumberOfTourists.Maximum = randAccum.NumberToCreate;
            tbar_rnd_MinNumberOfBadasses.Maximum = randAccum.NumberToCreate;
            if ( tbar_rnd_MaxNumberOfTourists.Value > randAccum.NumberToCreate ) {
                tbar_rnd_MaxNumberOfTourists.Value = randAccum.NumberToCreate;
            }
            if ( tbar_rnd_MinNumberOfBadasses.Value > randAccum.NumberToCreate ) {
                tbar_rnd_MinNumberOfBadasses.Value = randAccum.NumberToCreate;
            }
        }

        if ( control is CheckBox ) {
            CheckBox cbox = control as CheckBox;
            string controlName = cbox.Name.Remove ( 0, 8 );
            //because use Ratio is notted, we have to set it by hand.  
            //The For loop is left incase we add any more checkboxes.
            if ( controlName.Equals ( "useRatio" ) ) {
                tbar_rnd_FtMRatio.Enabled = !cbox.Checked;
                randAccum.useRatio = !cbox.Checked;
            }
            else {
                foreach ( var prop in randAccum.GetType( ).GetProperties( ) ) {
                    if ( controlName == prop.Name ) {
                        if ( prop.PropertyType == typeof(bool) ) {
                            prop.SetValue ( randAccum, cbox.Checked );
                        }
                    }
                }
            }
        }
        randAccum.NumberToCreate = randAccum.Pilots + randAccum.Engineers + randAccum.Scientists;
        UpdateRandomDisplay ( );
    }

And here's the accumulation class:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace KerbalTherapist.Accumulators {
public class RandomAccumulator {
    //number of kerbals to create
    public int NumberToCreate { get; set; }



    public bool useRatio { get; set; }

    public bool isKerman { get; set; }

    //gendersettings;
    public float FtMRatio { get; set; }
    //ability scores
    public float MinStupid { get; set; }

    public float MaxStupid { get; set; }

    public float MinBrave { get; set; }

    public float MaxBrave { get; set; }

    public int MinNumberOfBadasses { get; set; }

    public int MaxNumberOfTourists { get; set; }

    //Profession Ratios
    public int Pilots { get; set; }

    public int Engineers { get; set; }

    public int Scientists { get; set; }

    public RandomAccumulator ( ) {
        NumberToCreate = 1;
        FtMRatio = 0;
        useRatio = true; //needs to be true by default.
        MinStupid = 0.0f;
        MaxStupid = 0.0f;
        MinBrave = 0.0f;
        MaxBrave = 0.0f;
        MinNumberOfBadasses = 0;
        MaxNumberOfTourists = 0;

        Pilots = 0;
        Engineers = 0;
        Scientists = 0;
    }

    public RandomAccumulator Reset ( ) {
        return new RandomAccumulator ( );
    }
}

Answer 1

Quite frankly I'm not sure what your code fragment does, so I've reviewed your code in the way how can we improve legibility.

Split the UpdateRandomAccumulator method

• You can easily extract the TrackBar and Checkbox handling code into their own methods
• After that the UpdateRandomAccumulator method would like something like this

private void UpdateRandomAccumulator(object sender, EventArgs e)
{
    var control = sender as Control;
    if (control is TrackBar tbar)
    {
        HandleTrackbar(tbar, control.Name.Remove(0, 9));
    }
    else if (control is CheckBox cbox)
    {
        HandleCheckbox(cbox, cbox.Name.Remove(0, 8));
    }

    randAccum.NumberToCreate = randAccum.Pilots + randAccum.Engineers + randAccum.Scientists;
    UpdateRandomDisplay();
}

• Please be aware that this Name.Remove(x, y) is super fragile so try to avoid it if possible

The HandleTrackbar

• As I've stated in the first sentence without proper context it is impossible to know what your code really does
  • So, my suggestions here might be not applicable

static string[] ControlNames = { "MinNumberOfBadasses", "MaxNumberOfTourists" };
void HandleTrackbar(TrackBar tbar, string controlName)
{
    if (ControlNames.Contains(controlName) && tbar.Value >= randAccum.NumberToCreate)
    { 
        tbar.Value = randAccum.NumberToCreate;
    }

    var prop = randAccum.GetType().GetProperty(controlName);

    if (prop.PropertyType == typeof(float))
    {
        prop.SetValue(randAccum, tbar.Value / 100f);
    }
    else if (prop.PropertyType == typeof(int))
    {
        prop.SetValue(randAccum, tbar.Value);
    }
    else
    {
        Logger.LogEvent(prop.PropertyType.ToString());
    }
            
    tbar_rnd_MaxNumberOfTourists.Maximum = randAccum.NumberToCreate;
    tbar_rnd_MaxNumberOfTourists.Value = tbar_rnd_MaxNumberOfTourists.Value > randAccum.NumberToCreate
        ? randAccum.NumberToCreate
        : tbar_rnd_MaxNumberOfTourists.Value;

    tbar_rnd_MinNumberOfBadasses.Maximum = randAccum.NumberToCreate;
    tbar_rnd_MinNumberOfBadasses.Value = tbar_rnd_MinNumberOfBadasses.Value > randAccum.NumberToCreate
        ? randAccum.NumberToCreate
        : tbar_rnd_MinNumberOfBadasses.Value;
}

• I've replaced your control name check logic to a collection lookup
• As far I understand your code you don't need to get all properties, rather you are looking for a specific one based on the control name
  • So, I've removed your foreach loop and replaced your GetProperties to GetProperty call
• I've replaced your (float)tbar.Value / 100 to tbar.Value / 100f because the intent is more explicit here IMHO
• I've also replaced your guard expression for value assignments with ternary conditional operators

HandleCheckbox

void HandleCheckbox(ChechBox cbox, string controlName)
{
    if (controlName.Equals("useRatio"))
    {
        tbar_rnd_FtMRatio.Enabled = !cbox.Checked;
        randAccum.useRatio = !cbox.Checked;
        return;
    }

    var prop = randAccum.GetType().GetProperty("prop.Name");

    if (prop.PropertyType == typeof(bool))
    {
        prop.SetValue(randAccum, cbox.Checked);
    }
}

• More or less the same simplification as before
• +1: I've used early exit to avoid having else block

RandomAccumulator

• Please use consistent naming for your properties (NumberToCreate, useRatio, etc.)
• Set properties' default value on the auto-generated propreties rather than inside a dedicated ctor
  • Set the default value only if it differs from the runtime default

public class RandomAccumulator
{
    public int NumberToCreate { get; set; }

    public bool UseRatio { get; set; } = true;

    public bool IsKerman { get; set; }

    public float FtMRatio { get; set; }

    public float MinStupid { get; set; }

    public float MaxStupid { get; set; }

    public float MinBrave { get; set; }

    public float MaxBrave { get; set; }

    public int MinNumberOfBadasses { get; set; }

    public int MaxNumberOfTourists { get; set; }

    public int Pilots { get; set; }

    public int Engineers { get; set; }

    public int Scientists { get; set; }

    public RandomAccumulator Reset()
        => new();
}