1

I have a function that creates a new object by passing to it's constructor integer ids primarily with values of 0. This function is called when saving a newly created record (not an edit).

enter image description here

public class RecordController
{
    private int _recordId;
    private int _subRecordId;
    private string _xmlData;

    private SubRecordA CreateSubRecordA()
    {
        _xmlData = LoadXmlData();
        ParseXmlData();

        return new SubRecordA(
            SubRecordTypeId.A  // leaky DAL abstraction
            0,
            0,
            _xmlData);
    }
}

However, the two 0 parameters coincide with private fields that are already initialized to 0 in an Init() method

private void Init()
{
    _recordId = 0;
    _subRecordId = 0;
}

I'm not sure if it is better to explicitly pass these zero-valued fields to SubRecordA's constructor instead?

        return new SubRecordA(
            SubRecordTypeId.A  // leaky DAL abstraction
            _recordId,
            _subRecordId,
            _xmlData);

Passing the literal 0 seems more clear as to what the intention is, but also seems to weaken the logic by excluding these fields from it's flow (say for example _recordId gets initialized with a -1 at some point in the future).


Update

Using Robert Harvey's suggestion to let the default constructor zero out the properties/fields, I created an explicit constructor that takes only the required parameters and calls : this()

public class SubRecordA : SubRecord
{
    public override int SubRecordTypeId { get {return (int)SubRecordTypeId.A; } }
    public int ParentRecordId { get; set; }
    public int RecordId { get; set; }
    public string XmlData { get; set; }

    public SubRecordA(string xmlData) : this()
    {
        //SubRecordTypeId = SubRecordTypeId.A;
        XmlData = xmlData;
    }
}
samus
  • 475

1 Answers1

1

Your underlying problem is that 0 is an incorrect value for the id's

Ideally you would change them to GUIDs but a second best would be to change them to nullable uints.

This would allow you to have them set to null untill the object had been persisted and got actual ids

Ewan
  • 83,178