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
  • 465
  • 1
  • 5
  • 13
  • Are the zeros you are passing merely there to insure that `recordID` and `subRecordID` have some meaningful initial values? If that's the case, I'd say don't pass them at all. Just set them locally on construction. – Robert Harvey Apr 04 '18 at 15:52
  • Would you ever want to create a `SubRecordA` with nonzero values in those fields? – 1201ProgramAlarm Apr 04 '18 at 15:53
  • @RobertHarvey Yes, b/c they are actually substituted with temporary (local), negative values that are used as place holders until the records are synced. If there is an exception, the values (0) are restored anyway (it's kind of overkill, but I strive to maintain a consistent state as much as possible). – samus Apr 04 '18 at 15:54
  • What is the meaning of the negative value, and how is that different from zero? Do you do anything special in code with these negative values, like making decisions about program flow? – Robert Harvey Apr 04 '18 at 15:55
  • @RobertHarvey The id's are primary keys, so must be unique. Since connectivity to the remote DB isn't guaranteed, I need to create temporary values until the records are synced (inserted into main DB). – samus Apr 04 '18 at 15:58
  • 2
    OK, well I'm not convinced. The zeros can be automatically assigned at construction, so they don't need to be passed in. And unless the negative values have some special meaning, *you don't need them.* – Robert Harvey Apr 04 '18 at 15:59
  • @RobertHarvey But then I would have to call the default constructor, then manually assign the non-zero fields (ie _xmlData). – samus Apr 04 '18 at 16:01
  • If that's what it takes to construct a valid object, yes. – Robert Harvey Apr 04 '18 at 16:02
  • Just do what makes sense. You don't need a random stranger on the Internet to referee these kinds of design decisions, and there's no right or wrong answer except the one that best meets your specific requirements. – Robert Harvey Apr 04 '18 at 16:03
  • @RobertHarvey I guess that makes, to use default ctor and "override". But what if more than half, but less than all, the parameters are non-zero (jk)?. – samus Apr 04 '18 at 16:06
  • @1201ProgramAlarm The sub-record needs to be created and assinged to the parent record in the controller "layer" before passing it to a service layer below for storage. This service layer manages the actual id values. My object models (DB records primarily) themselves do not implement a repository interface, they are instead implemented as repository classes explicitly that the service layer consumes to read and write objects (the repos in turn consume a DAL, which implements a Query pattern and serializes objects with ADO - Entity wasn't available at the time, neither was SQLiteDatabase). – samus Apr 04 '18 at 16:14
  • @RobertHarvey I think a separate, explicit constructor that takes only the non-zero valued parameters and calls `: this()` would be the best of both worlds. Thanks! – samus Apr 04 '18 at 17:03
  • @1201ProgramAlarm You're right, I can just pass `xmlData` to the service layer with the parent record and construct the sub-record there with all non-zero params. – samus Apr 04 '18 at 18:33

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
  • 70,664
  • 5
  • 76
  • 161
  • I ended up moving the construction of the sub-record to the layer below, where it is persisted along with it's parent-record and all the constructor's parameters are known. The sub-record's `xmlData` is instead passed along with it's parent-record. – samus Apr 04 '18 at 19:09
  • Because the id's are primary keys, they need to be non-null and unique. The objects get serialzed into a local Sqlite DB that eventually gets synchronized to a remote SQL Server DB, so null/0 isn't an option. They appear in the OP's constructor, but are immediately replaced with temporary (negative) values when the object is serailzed during a call to a persistence layer. I refactored around this with the aforementioned solution above. – samus Apr 04 '18 at 19:18
  • 0 being an incorrect id value is a good point, and your proposed strategy does sound like an effective solution in dealing with that scenario. I am using guids in an auxiliary table of my own, but the main record tables are legacy and need updating. – samus Apr 04 '18 at 19:22