C# data class with public auto-implemented properties with constructor that specifies each

I have a dll and an executable which both deal in configuring certain things for a much broader software solution, and the code below in particular is directly meant for database configuration. (Here "inmate info" has been substituted for the actual configurations.)

In an MVC context, the dll is essentially the controller, and the executable is effectively the view. There is a data class - Inmate - which represents a central segment of the way data is configured in the database, and it is passed between the dll and the executable. There is a Windows forms class in the executable - InmateInfoForm - which inherits UserControl and is there for the user to specify how the database is configured in this regard.

Consider the following code in InmateInfoForm.cs:

```
private Inmate m_inmte;
internal Inmate Inmate
{
get
{
if (m_inmte == null)
{
m_inmte = new Inmate((int)NumericUpDownCriminalHistoryNumber.Value,
(int)NumericUpDownDownIntegrationID.Value,
m_eSecurityLevels[ComboBoxSecurityLevel.SelectedIndex],
RadioButtonIsInIsolation.Checked,
m_eDisciplinaryStatuses[ComboBoxDisciplinaryStatus.SelectedIndex],
(int)NumericUpDownAge.Value, TextBoxFirstName.Text,
TextBoxMiddleName.Text, TextBoxLastName.Text);
}
else
{
m_inmte.CriminalHistoryNumber = (int)NumericUpDownCriminalHistoryNumber.Value;
m_inmte.IntegrationID = (int)NumericUpDownIntegrationID.Value;
m_inmte.SecurityLevel = m_eSecurityLevels[ComboBoxSecurityLevel.SelectedIndex];
m_inmte.IsInIsolation = RadioButtonIsInIsolation.Checked;
m_inmte.DisciplinaryStatus =
m_eDisciplinaryStatuses[ComboBoxDisciplinaryStatus.SelectedIndex];
m_inmte.Age = (int)NumericUpDownAge.Value;
m_inmte.FirstName = TextBoxFirstNameFirstName.Text;

You have a constructor with nine parameters, all of them nullable. That's just asking for problems. Perhaps named arguments could be a solution, but in the context of your code I don't even see the point. Why not keep it simple, and do this:

if (m_inmte == null)
    {
        m_inmte = new Inmate();
    }

    m_inmte.CriminalHistoryNumber = (int)NumericUpDownCriminalHistoryNumber.Value;
    m_inmte.IntegrationID = (int)NumericUpDownIntegrationID.Value;
    m_inmte.SecurityLevel = m_eSecurityLevels[ComboBoxSecurityLevel.SelectedIndex];
    m_inmte.IsInIsolation = RadioButtonIsInIsolation.Checked;
    m_inmte.DisciplinaryStatus =
            m_eDisciplinaryStatuses[ComboBoxDisciplinaryStatus.SelectedIndex];
    m_inmte.Age = (int)NumericUpDownAge.Value;
    m_inmte.FirstName = TextBoxFirstNameFirstName.Text;
    m_inmte.MiddleName = TextBoxMiddleName.Text;
    m_inmte.LastName = TextBoxLastName.Text;

Unlike your current code you now only need to adapt one place if there is an extra parameter.

I'm worried by the fact that this Inmate is created in a get. At the very least I'd move that to a separate method, but quite frankly it looks like your UI and business logic is mixed together, which is another bad idea.

What is string?? A string is already nullable in C#, and I doubt string? is even legit.

There are some odd naming conventions in your code, like the prefixes m_ and p. Don't do that, it makes code so much harder to read.

Also avoid needless abbreviations like inmte.

Don't prefix form fields with their types, e.g. RadioButtonIsInIsolation or TextBoxFirstNameFirstName.

You don't check your input, e.g. m_inmte.CriminalHistoryNumber = (int)NumericUpDownCriminalHistoryNumber.Value;. What if an invalid value is entered into that text field?

StackExchange Code Review Q#126373, answer score: 4