intellij-community icon indicating copy to clipboard operation
intellij-community copied to clipboard

IDEA-290939 support more default values in code generation

Open entrypointkr opened this issue 2 years ago • 5 comments

Writing test case... do not merge this until additional commit! https://youtrack.jetbrains.com/issue/IDEA-290939

entrypointkr avatar Mar 24 '22 07:03 entrypointkr

Ready to get review @akozlova. Review points:

  1. Defaulting for Optional perfectly reasonable, then also good for Collection<A>, List<A>, even String?
  2. At this code, will find the default by PsiType#getCanonicalText. If not found, find again using PsiType#equalsToText. Is there a case that need equalsToText instead of getCanonicalText? if not, the equalsToText method need to be removed.

Thank you for your effort in advance.

entrypointkr avatar Mar 29 '22 16:03 entrypointkr

I don't understand when you would use these defaults actually. To me, in mostly 99% of cases these defaults would be overridden and it's better to have nothing than smth which pretend to be ok. Could you please elaborate why you need this? While the code is ok (see my comments). Thanks

akozlova avatar Mar 29 '22 16:03 akozlova

@akozlova The reason is same with Optional, as it basically a single size of List. The use case is, if we need kind of quick test to run a code without runtime error and redundant null check:

interface PersonRepository {
    List<Person> getAllPersons();
}
class SQLPersonRepository implements PersonRepository {  }

class EmptyPersonRepository implements PersonRepository {
    // Auto gen with defaults
{
PersonRepository repo = getRandomRepositoryWithReflection();
for (Person person : repo.getAllPersons()) {
    // at this case, commonly, we're not expecting the `getAllPersons()` will returns null.
}

Edit -- The reflection and quick test I'm not to meaning as it is a edge case. If we need such empty pattern like the EmptyPersonRepository above, and the functions in PersonRepository many in there, replacing nulls manually to empty will be annoying.

entrypointkr avatar Mar 29 '22 17:03 entrypointkr

Generally, it's making defaults more complicated. Optional was implemented because it's a known bad practice to return null from Optional method, there is no such problem with other types (at least other types were not invented to prevent nulls). Thus I generally don't like the change, I am sorry. One more argument against: if you insert Collections.emptyList(), you most probably insert import. If people would change the code (and most probably they would), then import would be unused and probably would be forgotten in the code (e.g. if optimize imports on commit is disabled). This way, these defaults would make the code worser. At the same time, I don't see big advantages - if you run test and fix the code - it's ok to fix NPEs as well as empty collections. Again, I am sorry for complaining in the PR but it would land for all users.

I think, it's possible to implement via template, do you want me to help you with it?

akozlova avatar Mar 29 '22 19:03 akozlova

@akozlova I agree with the import issue, even we can use optimize import, in many place it can being leaved as unused. Just remind about the argues,

The Optional implemented because nulling it is a known bad practice

  1. Then why is it a bad practice essentially? because Optional invented to replace null.
  2. Then why Optional invented? as Oracle says, avoid NPE and redundant null checking.

In this context, even if only Optional invented to avoid null, it's goal is actually to avoid NPE and redundant null checking. Then why we not treat List.empty() to avoid NPE and redundant checking? it's essentially a same. Reminding again:

List<Person> persons = personRepo.getAllPersons();
if (persons != null) {
    for (Person person : persons) {
        // TODO
    }
}

Insert Collections.emptyList() causes unused imports

Even it seems trivial, I 100% agree it is worse. Then we can List.of() on Java9+?


Please close the PR and issue still it not motivating to go.

entrypointkr avatar Mar 30 '22 10:03 entrypointkr