Запись в статическую переменную в методе экземпляра, почему это плохая практика?

Question

Я немного запутался здесь с этим предупреждением findbugs в затмении.

public class MyClass {
    public static String myString;
}


public class AnotherClass {
   public void doSomething() {
       MyClass.myString = "something";
   }
}

Это выдает мне предупреждение findbugs "запись в статическое поле из метода экземпляра", однако это не дает мне предупреждения:

public class MyClass {
    public static String myString;
}


public class AnotherClass {
   public void doSomething() {
       doAnotherThing();
   }
   public static doAnotherThing() {
       MyClass.myString = "something";
   }
}

Чем это отличается? И почему запись в статическую переменную из метода экземпляра является плохой практикой? Я предполагаю, что это связано с синхронизацией, но мне все еще неясно.

Я знаю, что похоже, что переменная должна быть окончательной, но я загружаю значение из файла свойств.

Answer 1

Это форма псевдонима, которая может быть нелогичной. Неинтуитивный код мешает простоте обслуживания.

Логически мы ожидаем, что методы экземпляра будут влиять на данные этого экземпляра. Мы ожидаем, что статические методы влияют на статические данные.

Давайте переименуем doSomething в initialize:

...
a.initialize();
...
b.initialize();
...

Читатель этого кода может не сразу понять, что экземпляры a и b фактически влияют на одни и те же данные. Это может быть ошибкой, поскольку мы инициализируем одну и ту же память дважды, но это неочевидно, поскольку кажется разумным, что нам может потребоваться вызывать initialize в каждом экземпляре.

Однако код был:

...
MyClass.initialize();
...
MyClass.initialize();
...

В этом случае более интуитивно понятно, что мы, вероятно, влияем на одни и те же статические данные, и это, скорее всего, ошибка.

Это похоже на обычную версию псевдонимов, где две переменные в одной области видимости указывают на один и тот же экземпляр.

---

Для вашего последнего примера,

• экземпляр вызывает статический метод

  Тот факт, что метод экземпляра вызывает статический метод, не должен вызывать флаги. Примеры, в которых это полезно, значительно перевешивают, где, вероятно, проблема.

• статический метод одного класса влияет на статические данные другого класса

  В каком-то смысле это должно сгенерировать другое, но похожее предупреждение: один класс связывается с данными другого класса. Однако, сделав статическую переменную общедоступной, это способ молчаливого утверждения этого, поэтому такое предупреждение не требуется.

Имейте в виду, что FindBugs просто пытается пометить потенциальные проблемы, а не все возможные проблемы, в вашем коде. Ваш первый пример, вероятно, является потенциальной проблемой обслуживания, которую вам нужно проверить, является ли это реальной проблемой. Ваш второй пример, скорее всего, не проблема, или это реальная проблема, которая слишком похожа на случаи использования, когда не проблема.

Answer 2

Существует не так много вариантов использования, почему вы хотите изменить статическое поле.Помните, что если вы установите для этого поля новое значение, это значение будет изменено для всех экземпляров этого класса.Это может привести к неприятностям в многопоточной среде, где несколько потоков вызывают doSomething().Требуется правильная синхронизация.

В 99% всех случаев вы хотите, чтобы методы вашего экземпляра изменяли только нестатические поля, поэтому findbugs предупреждает вас.

А findbugs недостаточно умен, чтобы узнать о методе вашего экземпляра, косвенно меняющем поле во втором примере:)

Answer 3

Вот что FindBugs должен сказать по этому поводу: http://findbugs.sourceforge.net/bugDescriptions.html#ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD

Answer 4

Это мой дубль, так что берите с зерном соли. Вы упомянули проблемы синхронизации, которые являются основной причиной этого предупреждения, но, что более важно, эти два случая в основном работают на разных концептуальных «уровнях» данных. Методы экземпляра «принадлежат» объектам и изменяют данные, которые описывают отдельные экземпляры. Методы класса являются общими операциями и утверждают, что, хотя и относятся к классу, они не связаны с отдельными объектами. Таким образом, изменение этого состояния изнутри каждого экземпляра, вероятно (но не обязательно), будет плохим дизайнерским решением.

Answer 5

Я не думаю, что синхронизация (упомянутая в нескольких ответах) имеет какое-либо отношение к этому.В конце концов, статические методы можно вызывать из нескольких потоков так же легко, как и методы экземпляров.

Причина предупреждения (не очень хорошо объясненная документацией FindBugs), на мой взгляд, подсказана паройиз ответов: это подозрительно и, возможно, ошибка.Как сказал Йохен Бедерсдорфер, не так уж много случаев использования, когда вы хотите назначить статическую переменную в одном классе из метода экземпляра в другом.Точно так же, как

while (x = y) {
    // ...
}

технически не является ошибкой (и фактически допустимой Java, если x и y являются логическими значениями), это почти всегда ошибка.Точно так же авторы FindBug чувствовали то же самое в отношении предмета дела.

Answer 6

Поскольку изменение статического поля изменяет его для всех экземпляров, вызывает неописанные проблемы, если они не синхронизированы должным образом.

Если вы читаете в файле свойств для установки общих полей, то делайте это статическим методом.В качестве альтернативы можно выполнить рефакторинг полей в отдельный одноэлементный экземпляр, из которого другой класс может только читать.Если у вас будет только один экземпляр, тогда используйте одноэлементный шаблон и сделайте поля нестатичными.

Статические методы должны влиять только на статические данные, а методы экземпляра должны влиять только на данные экземпляра.