2

I'm looking for some guidance. Is it a bad practice to use transformation functions within DTO objects?

I have this example

public partial class AgEmpDto
    {
        private DateTimeOffset _startDate;
        private DateTimeOffset? _endDate;

        public DateTimeOffset? LastUpdateDate { get; set; }
        public String LastUpdateUser { get; set; }
        public int? EmployeeId { get; set; }
        public int? InvestmentTeamId { get; set; }
        
        public DateTimeOffset StartDate
        {
            get => _startDate;
            set => _startDate = DateHelper.BuildStartDate(value);
        }

        public DateTimeOffset? EndDate
        {
            get => _endDate;
            set => _endDate = value != null ? DateHelper.BuildEndDate(value) : null;
        }
        public Int32 Id { get; set; }
    }

In order to set a value for StartDate and EndDate I need to format the value that I receive from the Angular Application to standard way for the Database. So I don't know whether putting those modifications within the DTO object is an antipattern.

5
  • 2
    I do this all the time. Because it has to map properly in the Dapper ORM, I place [Computed] attributes above these kind of properties, so that Dapper will ignore them. (Not Javascript, of course). Oct 13 at 15:28
  • Is this DTO passed in a request from the client? Is this DTO also mapped to the database directly, or is the class that maps to the database separate from this? Oct 14 at 19:39
  • Hello; Yes there is another class mapped to the database. This class is used for Front interaction and business operations.
    – Udemytur
    Oct 15 at 8:36
  • I need to format the value that I receive from the Angular Application to standard way for the Database Bearing in mind you are dealing solely with DateTimeOffset values (and not strings) I am not clear how any of this code will format anything. What exactly does BuildStartDate do?
    – John Wu
    Oct 18 at 18:38
  • Are we talking ASP.Net Web Api models here? Custom model binders seem more appropriate in such a scenario.
    – Flater
    Oct 22 at 10:07
3

On the non-technical side:

The fact that your frontend sends a date that is typed as DateTimeOffset (your setter) but that is not the values that you need for your datebase sounds like you are doing something wrong.

Given that neither your frontend nor your database should care about the format of dates, something else is going on here.

Your database should save absolute values. As the only source of truth on that matter, your frontend should send those absolute values to the backend.

The fact that you need to transform something with no side effects or additional data looks wrong to me. For example if you need to take into account time zones, you don't seem to actually pass the timezone the client is supposedly in. Better yet would be to agree on just sending UTC over the wire as "the truth" and not guess timezones in the backend as an afterthought.


On the technical side:

public DateTimeOffset? EndDate
{
    get => _endDate;
    set => _endDate = value != null ? DateHelper.BuildEndDate(value) : null;
}

That's horrible and breaks a few principles.

instance.EndDate = instance.EndDate;

This line should be close to a no-op. It should not change the end date at all. However, based on your code it very likely does. It's calling the transformation function again and again on the same value.

How about this:

public DateTimeOffset? EndDate { get; set; }

public DateTimeOffset? AngularFormattedEndDate
{
    set => this.EndDate = value != null ? DateHelper.BuildEndDate(value) : null;
}

At least now it is pretty clear that

instance.EndDate = instance.EndDate;

does nothing really and

instance.AngularFormattedEndDate = instance.EndDate;

is probably a programming mistake.

1
  • Comments are not for extended discussion; this conversation has been moved to chat.
    – maple_shaft
    Oct 25 at 13:29
2

a DTO does not have any behavior except for storage, retrieval, serialization and deserialization of its own data (mutators, accessors, parsers and serializers). In other words, DTOs are simple objects that should not contain any business logic but may contain serialization and deserialization mechanisms for transferring data over the wire.

Taken from: https://en.wikipedia.org/wiki/Data_transfer_object

In the example posted, the only logic is in the setter or getter. I don't see any additional business logic. So, I see no bad practices.

6
  • As a clarification, as long a BuildEndDate() doesn't have any side effect, I think we are fine. My assumption is that method/function is just formatting the date and nothing else.
    – Jon Raynor
    Oct 13 at 19:00
  • How much data transformation is Business logic though? Oct 19 at 21:43
  • The bad practice is that the DTO mutates its mutable properties. This introduces unexpected side effects. A plain data structure like a DTO should not introduce such behavior - this is bad in general. When you set a property via an accessible set(), then you'd expect it to return this exact value. In this context your postulated constraint "as long a BuildEndDate() doesn't have any side effect" is violated: BuildEndDate() introduces a serious unpredictable side effect via the public API.
    – BionicCode
    Oct 21 at 17:33
  • 1
    @AndreFigueiredo A DTO is allowed to transform itself (like the implementation of (de-)serialization logic) in order to execute transactions etc. For example converting itself to a JSON object would be fine - such logic does not involve any business rules. Data validation logic for example would violate this rule, because at this architectural level data validation is always the implementation of business rules.
    – BionicCode
    Oct 21 at 17:44
  • 1
    @AndreFigueiredo I agree. Business logic is a very generic term. Its definition must be re-defined for each application. You are right, the statement "I don't see any additional business logic" is very risky as we don't know the actual business. The conversion of date values, like in the question's example, could be indeed part of the application's business logic.
    – BionicCode
    Oct 21 at 19:51
2

No that's fine - think about it that way: if you'd exchange the database for another one the mapping logic will have to be exchanged too. That's a very strong sign, that this logic belongs to a persistence component.

One thing I'd consider changing - my guess is that the DateHelper also contains logic not associated with the db of your choice. Think about splitting that.

7
  • You are wrong. The bad practice here is that the DTO mutates its mutable properties. This introduces unexpected side effects. A plain data structure like a DTO should not introduce such behavior - this is bad in general. When you set a property via an accessible set(), then you'd expect it to return this exact value.
    – BionicCode
    Oct 21 at 17:35
  • If you propose to move it upwards - this is a very, very bad decision. You'd move persistence logic to a business logic component... and of course the getter should do the transformation the other way around. The implementation details do not need to be visible to the caller.
    – minime
    Oct 25 at 11:00
  • No it's not persitence logic. It's a simple data contract. The data entity should not convert it's own data. Rather must the client of hte service convert the data in order to make it compliant to the service interface. The client knows best about the original data and ithe meaning of e.g. data attributes. Otherwise you find external logic bleeding into your business or data layer.
    – BionicCode
    Oct 25 at 11:43
  • For example if a component uses a Rest service to retrieve all holidays, than this component must know how to handle the result data e.g. drop holidays for a cetain region i.e. only convert a certain data set. The data format of the service response can later change if the service changes or the response format (in case the version of th service changes). You don't want to have such details in your DTO. Otherwise you would have to modify your data entities only to accept a new input format/data in order to enable it to convert properly.
    – BionicCode
    Oct 25 at 11:43
  • The DTO should request an int and should not accept a string only to convert it. What if the source data changes to string? You can only handle/convert it if you change the DTO to accept the string and to give it knowledge about the formatting of this string. Not a robust design, not good. The current bad design is the result when you design your business logic after the services that it consumes, and adding service details to the application, what leads to instable design - as you can see. We design the entities first according to business rules. Then we provide interfaces and protocolls.
    – BionicCode
    Oct 25 at 11:44
0

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);
   }
 }

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.