You should never transform the data passed to a public property's set()
, especially when the instance doesn't return the original value from the property's get()
, of course.
It should be clear that such implementation introduces unexpected behavior (unexpected side effects). From that point of view, you should avoid modifying the stored public data implicitly i.e. avoid that the plain data structure mutates its own mutable properties. A mutable property is a property that allows its value to be changed via a public
/internal
or protected
property set()
method.
Your mistake is not about adding logic to the DTOs (as long it is not business logic). The mistake is that the logic silently modifies mutable properties and, in addition, the original value gets dropped.
A mutable property must always return the set value: "You get what you set".
Modifying the original property value should be only done via a method and wwhere the property is a read-only property.
Generally, a method is expected to have side effects - it either returns the result of the mutation or void
. We usually expect a void
method to have side effects. But this side effects should only impact public read-only properties (like the Count property of a collection) or private properties - but never properties with a accessible set()
.
The simple and clean solution is to convert the data BEFORE you pass it to the DTO: always construct a DTO with the proper (valid) data (data integrity).
Generally, validating or converting the data in the DTO is too late.
A DTO should not contain any such logic. Data validation is usually the implementation of business rules. Such logic must always be applied BEFORE you create the DTOs and preferebly before you send any data from the client to a service (you don't want to weaste resources by transferring invalid data). Usually your service would implement a protocoll that tries to make passing improperly formatted data to the service impossible.
At least you would have the service return an error response to force the client to format the data properly or to fix other data integrity issues.
This means convert the data on the client side (the Angular application) or when you create the DTOs. Or use transformation fuctions provided by the DBMS (database side).
But again, the client/service is responsible to ensure data integrity and not the DBMS or the DTOs (although the DBMS guards itself on data level by applying defined constraints like NOT NULL or verifying keys).
An exception exists where the DTO is responsible to save itself or executes transactions. In this case the DTO would know how to convert itself to the required format - but it would never change its own mutable properties for that.
Explaining the unexpected behavior
Assume you have an instance where you store a value in a mutable property e.g., a numeric text value:
// Assignment
instance.NumericText = "99";
You would always expect the value of the property not to change
// Expected behavior: successful parsing of the previously set value "99"
int numericValue = int.Parse(instance.NumericText);
Now, when the instance silently modifies it's mutable attributes e.g., by appending a alphabetic identifier to the original numeric text:
public string NumericText
{
get => this.numericText;
set => this.numericText = value + "A";
}
Then we have introduced an unexpected behavior:
// Will throw as the originally set NumericText is now suddenly alphanumeric
numericValue = int.Parse(instance.NumericText);
If mutating the stored public data is required, then provide a corresponding API or ensure that the mutable property returns the original value:
private string numericText;
// Make the property computed
public string NumericText
{
get => RemoveAlphabeticValueFromNumericText();
set => AppendAlphabeticValueToNumericText(value);
}
// Alternatively, make the converter methods part of the public API.
// Exp
public void AppendAlphabeticValueToNumericText(string value) => this.numericText = value + "A";
// Provide a method that reverses the internal mutation
public string RemoveAlphabeticValueFromNumericText() => this.numericText.Substring(0, this.numericText.length - 1);
Now that the above implementation provides a way to revert the converted value to the original value i.e. to hide the conversion from the public, the behavior of the property is as expected: "You get what you set".
This means in general, your implementation of a self mutating data object in regard of properties with a public setter, is bad practice, indeed. What makes it even worse is that you don't provide an API to get back the original value, either by introducing a dedicated property to hold the converted value or a conversion method.
If the class internally needs to operate on a transformed value, then consider to add a dedicated private property or a public property with a private setter (read-only) e.g.
public DateTimeOffset? FormattedEndDate { get; private set; }
public DateTimeOffset? EndDate
{
get => _endDate;
set
{
_endDate = value;
this.FormattedEndDate = DateHelper?.BuildEndDate(value);
}
}
[Computed]
attributes above these kind of properties, so that Dapper will ignore them. (Not Javascript, of course).DateTimeOffset
values (and not strings) I am not clear how any of this code will format anything. What exactly doesBuildStartDate
do?