Improve some toString methods in module domain API

[murdos]

No description provided.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f7fa4ad) to head (41af79a).
Report is 54 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##                main     #9372   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity      2941      2958   +17     
===========================================
  Files            734       734           
  Lines          12778     12810   +32     
  Branches         257       259    +2     
===========================================
+ Hits           12778     12810   +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gzsombor]

IMHO, this is not a good toString method:

1. This hides the type of the actual object - it's easy to think, that we have a plain String, however, this is something completely different
2. java records already have a toString method, which provides all the necessary information about the object: the type plus it's internal state.

[murdos]

This is perfectly acceptable, since this class/record a simple value object wrapper of a primitive (here: String).
When you do BigDecimal.valueOf(10).toString(), the string representation doesn't include "BigDecimal". This is similar.
Unwrapping the value in the toString() method brings simplicity and avoid clumsiness.
We already have that in existing code, e.g. VersionSlug, JHipsterModuleSlug, JHipsterDestination, ...

[gzsombor]

BigDecimal is not a simple wrapper around a String, it contains a BigInteger, a scale and a bunch of other values, just to implement a usable number - which quacks like a duck number. So if a bigDecimalObject.toString() equals to a doubleObject.toString(), the underlying bigDecimalObject and doubleObject can be used interchangeably. A toString is a method that converts the internal state into a String, a String that fully represents the internal state.

But these (VersionSlug,ProjectFolder,etc) wrapper classes exist for one reason: they represent, at the type level, the information that the internal content satisfies certain conditions, not just an arbitrary String but a String that is the key to a property, or path to a folder, or a version string. And if we see an x, where x.toString() returns "12.0", we won't know, if that "12.0" represents a folder, a version string, or a module name, or a ScriptCommand or just an user entered string without any validation. However, these are essential properties of the object x.

I know, this 'fake simplicity' pattern already used in this project, and I think, this is wrong, and should be avoided and this is one of the thing that annoys me.

[murdos]

TL;TR

The intent of a toString method is not to represent the internal state, but to provide "a concise but informative representation that is easy for a person to read".

The type (i.e. the class name) is not necessarily part of this representation, as demonstrated by several JDK classes (BigDecimal, BigInteger, Integer, Long, String, LocalDateTime, ...).

---

A toString is a method that converts the internal state into a String, a String that fully represents the internal state.

I disagree. It's your vision, but it's not the general contract of the toString method, which is: “a concise but informative representation that is easy for a person to read.”

For example BigDecimal toString method doesn't expose internal state, that consists of BigInteger intVal, int scale, int precision, String stringCache, ... instead it provides a concise but informative representation.
NB: There are very good reason to hide and do not expose internal state: they're not part of your API, and you want to be free to change your class implementation without breaking code that use it.

The Effective Java book has a dedicated chapter on the toString subject, and states: "providing a good toString implementation makes your class much more pleasant to use and makes systems using the class easier to debug".
This is exactly what I'm trying to do with the toString here:
JHipsterPropertyKey[key=myKey] is really not concise and pleasant since the class has only one property, thus you have a repetition of "key".

Compare the toString of JHipsterLandscape before and after this PR:

But these (VersionSlug,ProjectFolder,etc) wrapper classes exist for one reason: they represent, at the type level, the information that the internal content satisfies certain conditions, not just an arbitrary String but a String that is the key to a property, or path to a folder, or a version string.

Totally agree. These value objets provides strong typing (and this is principle of Type-Driven Development), but above all they are a much better expression of business/domain concepts than primitive types (such as String).

And if we see an x, where x.toString() returns "12.0", we won't know, if that "12.0" represents a folder, a version string, or a module name, or a ScriptCommand or just an user entered string without any validation. However, these are essential properties of the object x.

I disagree. The class name is not explicitly part of the toString contract.
And multiple jdk classes does not include it, and thus does not follow your logic:

• BigDecimal.valueOf(10).toString() => "10"
• new String("10").toString() => "10"
• Integer.valueOf(10).toString() => "10"
• Long.valueOf(10).toString() => "10"
• BigInteger.valueOf(10).toString() => "10"
• LocalDateTime.now().toString() => "2024-03-29T10:09:07.581832721"

@pascalgrimaud , @DamnClin : WDYT?

[gzsombor]

TL;TR

The intent of a toString method is not to represent the internal state, but to provide "a concise but informative representation that is easy for a person to read".

The type (i.e. the class name) is not necessarily part of this representation, as demonstrated by several JDK classes (BigDecimal, BigInteger, Integer, Long, String, LocalDateTime, ...).

A toString is a method that converts the internal state into a String, a String that fully represents the internal state.

I disagree. It's your vision, but it's not the general contract of the toString method, which is: “a concise but informative representation that is easy for a person to read.”

That's two very similar definition, just the later is more subjective.

For example BigDecimal toString method doesn't expose internal state, that consists of BigInteger intVal, int scale, int precision, String stringCache, ... instead it provides a concise but informative representation. NB: There are very good reason to hide and do not expose internal state: they're not part of your API, and you want to be free to change your class implementation without breaking code that use it.

The Effective Java book has a dedicated chapter on the toString subject, and states: "providing a good toString implementation makes your class much more pleasant to use and makes systems using the class easier to debug". This is exactly what I'm trying to do with the toString here: JHipsterPropertyKey[key=myKey] is really not concise and pleasant since the class has only one property, thus you have a repetition of "key".

Compare the toString of JHipsterLandscape before and after this PR:

But these (VersionSlug,ProjectFolder,etc) wrapper classes exist for one reason: they represent, at the type level, the information that the internal content satisfies certain conditions, not just an arbitrary String but a String that is the key to a property, or path to a folder, or a version string.

Totally agree. These value objets provides strong typing (and this is principle of Type-Driven Development), but above all they are a much better expression of business/domain concepts than primitive types (such as String).

And if we see an x, where x.toString() returns "12.0", we won't know, if that "12.0" represents a folder, a version string, or a module name, or a ScriptCommand or just an user entered string without any validation. However, these are essential properties of the object x.

I disagree. The class name is not explicitly part of the toString contract. And multiple jdk classes does not include it, and thus does not follow your logic:

I never said that the classname should be part of the toString - specially for these very verbosly named classes, I would rather see:
PropertyKey[myKey], Version[3.2], Folder[something], Tag[mytag]

So JHipsterModuleResource could become something like this:
`ModuleResource[Slug[my-slug],ApiDoc[Group[org.jhipster], Operation[something]],....

[murdos]

So JHipsterModuleResource could become something like this: `ModuleResource[Slug[my-slug],ApiDoc[Group[org.jhipster], Operation[something]],....

That's a bit unusual.
And not using the full name could be misleading: e.g. both PropertyKey and JHipsterPropertyKey exit.

Also I don't understand how you would deal with a class that would have multiple fields with the same type, e.g. if JHipsterModuleResource had JHipsterModuleSlug slug and JHipsterModuleSlug parentSlug.
ModuleResource[Slug[my-slug1],Slug[my-slug2],ApiDoc[...]], how would you know which one is the parentSlug?

[gzsombor]

So JHipsterModuleResource could become something like this: `ModuleResource[Slug[my-slug],ApiDoc[Group[org.jhipster], Operation[something]],....

That's a bit unusual. And not using the full name could be misleading: e.g. both PropertyKey and JHipsterPropertyKey exit.

It's much less misleading to see PropertyKey[abc], instead of just abc. But JHipsterPropertyKey[abc] could be fine as well for me, in this case.

Also I don't understand how you would deal with a class that would have multiple fields with the same type, e.g. if JHipsterModuleResource had JHipsterModuleSlug slug and JHipsterModuleSlug parentSlug. ModuleResource[Slug[my-slug1],Slug[my-slug2],ApiDoc[...]], how would you know which one is the parentSlug?

If there are more than 1 slugs in a class, we can prefix it with the name of the field (just like when we have more than 1 Strings, we could do the same).
So in this case: ModuleResource[slug=Slug[my-slug1],parent=Slug[my-slug2]...

[DamnClin]

For me, simple wrapper Value Objects should toString() the value.

It's another debate for VO with multiple fields but for the ones with only one field it's easier to use since we can append the VO in a string without null check and but null safe (pretty handy for exception messages for example)