3
\$\begingroup\$

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;
            m_inmte.MiddleName = TextBoxMiddleName.Text;
            m_inmte.LastName = TextBoxLastName.Text;
        }
        return m_inmte;
    }

and the following code in Inmate.cs:

public int? CriminalHistoryNumber { get; set; }
public int? IntegrationID { get; set; }
public SecurityLevel? SecurityLevel { get; set; }
public bool? IsInIsolation { get; set; }
public DisciplinaryStatus? DisciplinaryStatus { get; set; }
public int? Age { get; set; }
public string FirstName { get; set; }
public string MiddleName { get; set; }
public string LastName { get; set; }

public InmateInfo(int? pCriminalHistoryNumber, int? pIntegrationID, SecurityLevel?
        pSecurityLevel, bool? pIsInIsolation, DisciplinaryStatus? pDisciplinaryStatus, int?
        pAge, string pFirstName, string pMiddleName, string pLastName)
{
    CriminalHistoryNumber = pCriminalHistoryNumber;
    IntegrationID = pIntegrationID;
    SecurityLevel = pSecurityLevel;
    IsInIsolation = pIsInIsolation;
    DisciplinaryStatus = pDisciplinaryStatus;
    Age = pAge;
    FirstName = pFirstName;
    MiddleName = pMiddleName;
    LastName = pLastName;
}

public static Inmate CreateDefaultData(EnumType3 pEnumType3Value, EnumType4
        pEnumType4Value)
{
    // some code
}

In the getter in InmateInfoForm, when m_inmte is equal to null, is it generally better form to leave the code as-is, to go ahead and create a default constructor and use it like this:

        if (m_inmte == null)
        {
            m_inmte = new Inmate();
        }
        else
        {
            m_inmte.CriminalHistoryNumber = (int)NumericUpDownCriminalHistoryNumber.Value;
            m_inmte.IntegrationID = (int)NumericUpDownDownIntegrationID.Value;

The first form shaves off an unnecessary bit of interface and is slightly more direct/efficient, but the second form helps prevent having to put things like (int)NumericUpDownCriminalHistoryNumber.Value (something trivial) in two different places, and it also looks a little bit neater. I included the code about the factory method in Inmate just to say that it's there, but it's not particularly useful in the getter inside of InmateInfoForm's case. Also the current constructor (which specifies all the properties) is in use inside of some other code that does not run into the scenario above.

Which is generally better form to use and why?


Notes:

The Inmate class is being gotten from the UI class as a means of showing the user's input and keeping it all in one place, so it can be passed around as one reference. (Is the way in which this is being implemented bad?)

Some of the code here is not the exact code in use and lacks some of the context of outside code (e.g., for using nullable types). The UI prefixes would be abbreviated in production (e.g. NumericUpDownSomething => nudSomething).

The structure of the code above is quite close to what's actually there.

UPDATE

This answer to a more recent question on Programmers.SE is somewhat linked.

\$\endgroup\$
  • \$\begingroup\$ It is just me, but I do not like having nullable types in my code. I understand the database can return null values, but I think it is better practice for the class to be written to handle those null values and write back to the database accordingly. \$\endgroup\$ – jp2code Apr 22 '16 at 13:33
  • 1
    \$\begingroup\$ Also, you mentioned using m_inmte.CriminalHistoryNumber = (int)TextBoxCriminalHistoryNumber.Text in your getter. If that TextBox value is empty or some bozo put "foobar" in the field, that getter will fail. Properties are not designed to handle exceptions, so your program may never know it happened. It just fails. \$\endgroup\$ – jp2code Apr 22 '16 at 13:38
  • \$\begingroup\$ @jp2code Lol, thanks, I guess I should've been a little more careful when switching their types around. \$\endgroup\$ – Panzercrisis Apr 22 '16 at 14:10
  • \$\begingroup\$ What are the DisciplinaryStatus and SecurityLevel types? Are they enums, structs or classes? \$\endgroup\$ – Greg Burghardt Apr 22 '16 at 14:43
  • \$\begingroup\$ @GregBurghardt Enums. \$\endgroup\$ – Panzercrisis Apr 22 '16 at 14:45
4
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ I started commenting on this and just gave up. +1 for all good points. \$\endgroup\$ – Mattias Åslund Apr 22 '16 at 14:57
  • \$\begingroup\$ Thanks, BCdotWEB! If the class is also generated using data from the database, does that weigh on whether to provide both constructors, or should everything, including in the controller, just stick to the default constructor and set everything afterwards? \$\endgroup\$ – Panzercrisis Apr 22 '16 at 15:14

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.