hubert
41 уровень

Говнокод #1

Статья из группы Архив info.javarush
участников
boolean is_admin; // something Boolean b = new Boolean( is_admin ); if( b.toString().length() == 4 ) { // something... } // something
Комментарии (28)
  • популярные
  • новые
  • старые
Для того, чтобы оставить комментарий Вы должны авторизоваться
IvanDurov
Уровень 25
20 апреля 2014, 19:55
return 5 < age ? false : true

Правильно:
return age <= 5
alexnjc
Уровень 31
20 апреля 2014, 03:49
boolean is_admin;
// something
Boolean b = new Boolean( is_admin );
if( b.toString().length() == 4 ) {
   // something...
}
// something

Поскольку b.toString().length() == 4
соответствует только true и null, но из примитивного boolean нельзя получить null.
Все упрощается так:
boolean is_admin;
// something
if(is_admin) {
   // something...
}
// something
Predict_it
Уровень 21
20 апреля 2014, 02:29
int var;
int var2;
int var3;
int var4;

int var5 = var + var2 + var3 + var4;
L2CCCP
Уровень 9
19 апреля 2014, 02:21
public class Access
{
	private boolean isAdmin = false; //Default is false

	public void setAdmin(boolean isAdmin)
	{
		this.isAdmin = isAdmin;
	}

	public boolean getAdmin()
	{
		return isAdmin;
	}

	public void doSomething()
	{
		if(getAdmin())
		{
			//do something...
		}
	}
}
Timur
Уровень 20
19 апреля 2014, 16:20
Я поставил минус так как если бы я использовал твой класс то в итоге было бы что-то типо этого:

...
Access access = new Access(); // создаем доступ
access.setAdmin(true); // устанавливаем доступ админом
boolean admin = access.getAdmin(); // получаем админа

...     

// если получаем админа
if (access.getAdmin()) {
    // что-то делаем
}
...

access.setAdmin(false); // устанавливаем доступ НЕ админом
boolean admin2 = access.getAdmin(); // получаем НЕ админа
...


Если я вызываю например у объекта Button методы:

button.getText(); // я как правило ожидаю в ответ какой-либо String.
button.setText("blabla"); // я ожидаю что у объекта установиться мой String.
button.isEnabled(); // я ожидаю что объект вернет мне boolean.


Если я не правильно юзаю твой код, то интересно было бы узнать как нужно правильно.
L2CCCP
Уровень 9
19 апреля 2014, 18:38
Если Вы слышали о таком принципе ООП как инкапсуляция Вы поймете мой пример :)
Groomsh
Уровень 33
19 апреля 2014, 18:57
А зачем эта строка, если не секрет?)
В смысле — зачем явное указание, учитывая, что есть дефолтная инициализация переменных?)
private boolean isAdmin = false; //Default is false
Timur
Уровень 20
19 апреля 2014, 19:18
Наверно из-за того что во многих книгах советуют явно в некоторых случаях инициализировать переменные для лучшей читабельности например
счетчик советуют всегда инициализировать явно.
Timur
Уровень 20
19 апреля 2014, 19:21
Я слышал и прочитал не одну книгу и смотрел много всяких исходников поэтому я и спросил) Тут вопрос насчет читабельности а не инкапсуляции мне кажется если сделать хотя бы из getAdmin() -> isAdmin() и подумать еще над именем класса читаймость бы увеличилась.

Вы поймете мой пример :)
после чтение кода да а если я не хочу лезть в ваш класс а хочу просто его использовать а не тратить свое время на его изучение?
L2CCCP
Уровень 9
20 апреля 2014, 00:00
Суть моего примера была в использовании сеттеров и геттеров для переменных приватного типа по принципу инкапсуляции.

Вот немного измененный вариант вашего примера с использованием сути которой я хотел донести.


public class User
{
	public static void main(String[] args)
	{
		User one = createUser("Timur", Role.USER);
		User two = createUser("L2CCCP", Role.ADMIN);

		System.out.println("У пользователя " + one.getName() + " права " + one.getRole());
		System.out.println("У пользователя " + two.getName() + " права " + two.getRole());
		System.out.println("------------------------------------");
		System.out.println("-----------Изменяем права-----------");
		System.out.println("------------------------------------");
		one.setRole(Role.ADMIN);
		two.setRole(Role.USER);
		System.out.println("У пользователя " + one.getName() + " права " + one.getRole());
		System.out.println("У пользователя " + two.getName() + " права " + two.getRole());
	}

	private String name;
	private Role role;

	private User(String name, Role role)
	{
		this.name = name;
		this.role = role;
	}

	public static User createUser(String name, Role role)
	{
		return new User(name, role);
	}

	public void setRole(Role role)
	{
		this.role = role;
	}

	public Role getRole()
	{
		return role;
	}

	public void setName(String name)
	{
		this.name = name;
	}

	public String getName()
	{
		return name;
	}

	public enum Role
	{
		USER,
		ADMIN
	}
}

Использовать данный код удобней как на мой взгляд.

А на счет дефолтной инициализации, дело не в книгах, а в привычке так как если Вы писали высоконагруженные приложения то должны знать что инициализацию нужно делать для избежания ошибок при изъятии данных от Объектов.

Надеюсь ответи
Timur
Уровень 20
20 апреля 2014, 06:39
Использовать данный код удобней как на мой взгляд.

Да для простоты примера я не стал добавлять Getters, Setters, toString, Comparable и т.д. хотя можно было бы.

Но мое замечание было насчет читаймости вашего примера а не его инкапсуляции)

А на счет дефолтной инициализации, дело не в книгах, а в привычке так как если Вы писали высоконагруженные приложения то должны знать что инициализацию нужно делать для избежания ошибок при изъятии данных от Объектов.

А можно пруф? Так как в данном примере:


...
private boolean isAdmin = false; //Default is false
...


boolean — примитивный тип данных и Java гарантирует нам что при при изъятии там будет false если мы даже явно не инициализируем данную переменную. Какие ошибки вы имеете ввиду?
Timur
Уровень 20
18 апреля 2014, 19:20

public class User {
    private String name;
    private Role role;

    private User(String name, Role role) {
        this.name = name;
        this.role = role;
    }

    public static User createUser(String name, Role role) {
        return new User(name, role);
    }

    public boolean isRoleAdmin() {
        return role == Role.ADMIN;
    }

    public enum Role {USER, ADMIN}
}
KeLsTaR
Уровень 24
22 апреля 2014, 01:11
Прямо без проверки корректности имени? На null проверить хотя бы.

И мне не понятен смысл приватного конструктора и вызов его же через отдельный публичный метод, просветите.
Timur
Уровень 20
22 апреля 2014, 16:12
Прямо без проверки корректности имени? На null проверить хотя бы.
Да проверку на null стоило бы добавить. Насчет корректности тут уже зависит от требований.


if (name == null)
    throw new IllegalArgumentException("Some message ...");

Или можно также подключить Lombok

И мне не понятен смысл приватного конструктора и вызов его же через отдельный публичный метод, просветите.
Effective Java 2nd Edition, Item 1: Consider static factory methods instead of constructors
KeLsTaR
Уровень 24
23 апреля 2014, 08:01
Мне кажется, ты не верно понял суть такого подхода с конструкторами. Там говорится о том, что статические методы удобнее конструкторов когда либо у тебя есть много одинаковых конструкторов, отличающихся только списком параметров(лучше оставить 1 конструктор и методы ссылать на него), либо ты не хочешь всегда возвращать новый объект (синглон там, или енамы), либо тебе надо возвращать объект подкласса (как в классе Calendar), либо тебе нужно как-то назвать конструктор, чтобы он имел более очевидный результат выполнения. В этом коде нету ничего, чему статический метод мог бы помочь, он у тебя просто переводит в конструктор все, но при этом есть большой минус — от такого юзера ты уже не сможешь наследоваться.
Timur
Уровень 20
23 апреля 2014, 17:16
Ну обычно у юзеров несколько ролей например простые юзеры, модераторы, супермодераторы, тех. поддержка, админы и может понадобиться создать несколько методов createUser, createModerator, createSuperModerator, createAdmin… isRoleModerator,…
в свою очередь у каждого могут быть разные свойства. В зависимости от задачи ты дальше будешь решать как тебе изменить класс может ты решишь прописать свойства в Role enum может выделить для этого другой класс и т.д. и т.п.

Так как тут нет конкретной задачи я предложил свой вариант ты можешь улучшить его или написать свой. #comment16216 тут я уже писал что не стал включать геттеры и остальное.

есть большой минус — от такого юзера ты уже не сможешь наследоваться.

А дальше ты читал?

The main disadvantage of providing only static factory methods is that classes without public or protected constructors cannot be subclassed.

Arguably this can be a blessing in disguise, as it encourages programmers to use composition instead of inheritance (Item 16).


www.artima.com/lejava/articles/designprinciples4.html
Item16: Favor composition over inheritance

ну если тебе прям так надо ты можешь изменить private на protected
max
Уровень 30
18 апреля 2014, 16:54
// something…

))
terranum
Уровень 28
18 апреля 2014, 17:00
// anything...
terranum
Уровень 28
18 апреля 2014, 17:00
Да, я украл твою идею)
terranum
Уровень 28
18 апреля 2014, 16:40

// something
if(;;) {
   // something...
}
// something
SergeyKandalintsev
Уровень 32
18 апреля 2014, 16:43
ШТОАААААА!!?
terranum
Уровень 28
18 апреля 2014, 16:47
Это пример кода похуже)))
SergeyKandalintsev
Уровень 32
18 апреля 2014, 16:49
начнем с того что задача топика код улучшать, закончим тем что это совсем не код…
terranum
Уровень 28
18 апреля 2014, 16:50
Никогда так не делайте!
terranum
Уровень 28
18 апреля 2014, 16:50
ок, кончай
Sant9Iga
Уровень 41
18 апреля 2014, 13:02
boolean is_admin;
// something
        
        if( is_admin) {
            // something...
        }
// something
Izuver
Уровень 31
18 апреля 2014, 21:56
Мне кажется, что стоит также изменить имя boolean переменной на admin. Сдвигать if табулятором тоже не стоило. Поправьте, если не прав.
gram2005
Уровень 30
18 апреля 2014, 22:31
Тогда уж лучше CamelCase: isAdmin