Checker ImproperField

belongs to group Basic
Identify fields that should be replaced by local variables or made final

Frameworks supported by this checker

  • java up to 11
  • android up to API level 28
  • dotnet

Warnings generated by this checker

  • FieldIsOnlyUsedInConstructorsWarning: a field is only used in a constructor and could hence be replaced by a local variable [ CWE772 ]
  • FieldIsOnlyUsedInStaticInitialiserWarning: a field is only used inside a static initializer and could hence be replaced by a local variable [ CWE772 ]
  • FieldShouldBeReplacedByLocalsWarning: a field should be replaced by local variables inside the methods that use it [ CWE772 ]
  • MutableEnumWarning: an enumeration can be muted [ CWE607 ]
  • UselessFieldUpdateWarning: a field is updated but the written value is never used later [ CWE563 ]

Options accepted by this checker

  • none

Annotations understood by this checker

  • none


Description

Fields should be used to hold the state of the objects. They are not meant to hold temporary data that is not used again after a method completes. In that case, we talk about improper fields, that should rather be replaced by variables local to the methods. Improper fields complicate the code since they make it messy. They cost memory, since they require space for themselves; moreover, if they have reference type, they prevent the garbage collection of the memory reachable from them.

Action: Use variables local to the methods and the constructors instead of fields.

Fields should be used to hold the state of the objects. They are not meant to hold temporary data that is not used again after a method completes. In that case, we talk about improper fields, that should rather be replaced by variables local to the methods. Improper fields complicate the code since they make it messy. They cost memory, since they require space for themselves; moreover, if they have reference type, they prevent the garbage collection of the memory reachable from them.

Action: Use variables local to the methods and the constructors instead of fields.

Examples

Consider the following program:

public class ImproperField {
  private int[] array = new int[100];
  private int counter;
  private static int id;
  public String TAG = "MY_TAG" + id++;
  public String prefix = TAG + ": ";

  public void init() {
    for (counter = 0; counter < array.length; counter++)
      array[counter] = counter;
  }

  public String getError(String message) {
    return prefix + message;
  }

  public void clear() {
    for (counter = 0; counter < array.length; counter++)
      array[counter] = 0;

    counter = 0;
  }
}

This checker issues the following warnings:

ImproperField.java: [ImproperField: FieldIsOnlyUsedInConstructorsWarning] Field ImproperField.TAG is only used in constructors: make it final or use local variables
ImproperField.java: [ImproperField: FieldShouldBeReplacedByLocalsWarning] Field ImproperField.counter should be replaced by local variables
ImproperField.java:21: [ImproperField: UselessFieldUpdateWarning] This assignment to field ImproperField.counter seems useless: the value written inside the field seems never used

since field counter is assigned inside each method that uses it, so it is improperly used instead of local variables. As a consequence, also the assignment at line 21 is useless. Moreover, field TAG should be final since it is only used by the constructor of ImproperField. The assignment at line 21 should be removed:

public class ImproperField {
  private int[] array = new int[100];
  private static int id;
  public final String TAG = "MY_TAG" + id++;
  public String prefix = TAG + ": ";

  public void init() {
    for (int counter = 0; counter < array.length; counter++)
      array[counter] = counter;
  }

  public String getError(String message) {
    return prefix + message;
  }

  public void clear() {
    for (int counter = 0; counter < array.length; counter++)
      array[counter] = 0;
  }
}

Consider the following program:

namespace DocumentationExamples
{

    public class ImproperField
    {
        public static void Main(string[] args)
        { }

        private int[] array = new int[100];
        private int counter;

        public void Clear()
        {
            for (counter = 0; counter < array.Length; counter++)
                array[counter] = 1;
            counter = 0;
        }
    }
}

This checker issues the following warnings:

DocumentationExamples.cs: [ImproperField: FieldShouldBeReplacedByLocalsWarning] Field "counter" should be replaced by local variables
DocumentationExamples.cs:16: [ImproperField: UselessFieldUpdateWarning] This assignment to field "counter" seems useless: the value written inside the field seems never used

since field counter is assigned inside each method that uses it, so it is improperly used instead of local variables. As a consequence, also the assignment at line 16 is useless.

namespace DocumentationExamples
{

    public class ImproperField
    {
        public static void Main(string[] args)
        { }

        private int[] array = new int[100];

        public void Clear()
        {
            for (int counter = 0; counter < array.Length; counter++)
                array[counter] = 1;
        }
    }
}