Deprecated annotation best practice

Hi devs,

This tweet https://twitter.com/piotrprz/status/1343866413107310594 prompted me to check our documented practice for deprecated and I’ve noticed that we don’t have a best practice documented for @Deprecated on https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/JavaCodeStyle/, so here’s a proposal:

Before we move to Java 9+

Rules:

  • Always use both @Deprecated annotation and the @deprecated javadoc tag.
  • In the @deprecated javadoc tag, always specify since WHICH version it’s deprecated, and WHAT should be used instead. Also explain WHY it’s deprecated.

Examples:

  • Good:
    • Couldn’t find any example in xwiki-platform! It should specify the 3 elements (version, what, why).
  • Partially good (missing WHY):
      /**
       * The hierarchy of Entity Types.
       * 
       * @deprecated since 10.6RC1, use {@link EntityType#getAllowedParents()} instead
       */
      @Deprecated
      Map<EntityType, List<EntityType>> PARENT_TYPES = new EnumMap<EntityType, List<EntityType>>(EntityType.class)
    
  • Bad (missing version):
     /**
      * @return the first form contained in this section.
      * @deprecated this method can be ambiguous since an administration section page might contain several forms.
      *              The method {@link #getFormContainerElement(String)} should be used instead.
      */
     @Deprecated
     public FormContainerElement getFormContainerElement()
     {
         return new FormContainerElement(this.formContainer);
     }
    
  • Bad:
    @Deprecated
    public class i18n
    

After we move to Java 9

Rules:

  • Same idea of the 3 elements that must be specified.
  • However the @Deprecated annotation now support 2 parameters and they must both be used with forRemoval always set to true and since should specify the version.
  • The javadoc tag must still be there to explain the WHY and the WHAT to use.

Good example:

public class Worker {
    /**
     * Calculate period between versions
     * @deprecated This method is no longer acceptable to compute time between versions because...
     * Use {@link Utils#calculatePeriod(Machine)} instead.
     *
     * @param machine instance
     * @return computed time
     */
    @Deprecated(since = "4.5", forRemoval = true)
    public int calculate(Machine machine) {
        return machine.exportVersions().size() * 10;
    }
}

WDYT?

Thanks

1 Like

I think this is an good solution.

Globally +1 for the deprecated with the 3 infos.

I’m not sure about this: whenever we moved a deprecated method / class in legacy we consider it as “removed” and it will stay like this forever basically with the deprecated annotation. So the semantic of the “forRemoval” seems a bit wrong there for me.

ah yes good point. So it’s the opposite, we must never use forRemoval (default is false).

Pretty much copy/paste of @surli’s message.