(Спойлер) Нужно мнение экспертов, на сколько криво я решил задачу

Вопрос по задаче

Обсуждается

Решал по условиям, сначала выполнил первые два и начал тонуть на последних двух. Перебирал вариации с ifами. И вот РЕШЕНО! Но после решения посмотрев "Правильное решение" закралось сомнение, что уш больно криво я все сделал. Вопрос на сколько рационально мое решение указанной задачи? public static void paySalary(String name) { if (waitingEmployees.contains(name) == false) { } else { alreadyGotSalaryEmployees.addAll(waitingEmployees); for (int i = 0; i < alreadyGotSalaryEmployees.size(); i++) { System.out.println(alreadyGotSalaryEmployees.get(i)); } for (int i = 0; i < waitingEmployees.size(); i++) { waitingEmployees.set(i, null); System.out.println(waitingEmployees.get(i)); } } } }

Комментарии (7)

популярные
новые
старые

Для того, чтобы оставить комментарий Вы должны авторизоваться

hidden #598481

Уровень 27

18 октября 2022, 14:53

Пустых ифов быть не должно. Сравнения с булевыми тоже. Это сразу признаки плохого кода. Здесь абсолютно согласен с Денисом, могу только добавить пару деталей. Либо мы просто весь имеющий логику код оборачиваем в один иф:

if (waitingEmployees.contains(name)) {
    // logic here
}

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

if (!waitingEmployees.contains(name)) // "!" инвертирует условие
    return; // прерываем метод

// logic here

Команда return однозначно скажет о том. что ничего не произойдет, если выполнилось условие в скобках. В твоём коде это не так очевидно, потому что стоит тебе ошибиться в расстановке отступов, как может оказаться, что после всего блока if-else есть какой-то код, который выполнится независимо от условия. Поэтому если хочешь действительно прервать метод, используй return. Как говорится, "уходя, уходи".

hidden #598481

Уровень 27

18 октября 2022, 15:01

Что касается логики работы остального кода. Если честно, я вовсе не уверен, что она корректная. Насколько я помню, тебе нужно обнулить наличие конкретного сотрудника в одном списке и добавить его в другой. Почему тогда получается так, что ты всех сотрудников переводишь из "ожидающих" в "получивших", даже ни разу не сверившись с тем, кого как зовут? Исходя даже из названия метода и его аргумента, ты должен выплатить зарплату одному конкретному сотруднику, а не всему штату. Если речь про одного конкретного, то отсюда мы делаем вывод, что циклы нам не нужны. Зачем цикл, если надо одного обнулить и одного добавить? Даже если ты добавишь внутрь цикла проверку ифом и брейк после, то это будет говорить лишь о том, что ты используешь цикл там, где он не нужен, и в конечном итоге правишь только одну строку. То есть нужно обойтись без цикла, найдя нужного сотрудника и проделав необходимые операции. Вывод в этом методе тоже не нужен, т.к. если метод называется pay, то он должен платить, а не печатать. Да и в выводе посреди алгоритма нет никакого смысла: для того, чтобы посмотреть текущее состояние списков есть дебаг, а если требуется что-то вывести, то делаешь отдельный метод и в нём выводишь, что требуется. У тебя же вывод происходит при каждой выплате зарплаты: то есть если я хочу выплатить зарплату пяти сотрудникам, я пять раз вызову метод paySalary и пять раз получу вывод обоих списков на экран. А оно мне зачем? Резюмируя. Твой код не рабочий, ты по запаре отправил на проверку код из правильного решения и сам того не понял.

W18

Уровень 19

20 октября 2022, 11:22

Большое Вам спасибо!

Денис Enterprise Java Developer

18 октября 2022, 14:43

Ну давай разбираться:

public static void paySalary(String name) {
        if (waitingEmployees.contains(name) == false) {
        } else {
            alreadyGotSalaryEmployees.addAll(waitingEmployees);
            for (int i = 0; i < alreadyGotSalaryEmployees.size(); i++) {
                System.out.println(alreadyGotSalaryEmployees.get(i));
            }
            for (int i = 0; i < waitingEmployees.size(); i++) {
                waitingEmployees.set(i, null);
                System.out.println(waitingEmployees.get(i));
            }
        }
    }
}

Вторая строка if (waitingEmployees.contains(name) == false) Эквивалетно записи: Если Масло маслянное то... if (waitingEmployees.contains(name)) вот так же проще гораздо? Оно уже возвращает булево значение. Да и вообще что за странное условие? у тебя пустой блок после if, так зачем тебе проверять это условие? Инвертируй его, и перенеси блок кода из else Вот эта строка тоже смущает: четвертая строка - alreadyGotSalaryEmployees.addAll(waitingEmployees); А что если у тебя дальше что-то пойдет не так и зарплату ты выдашь не всем? логично такие вещи писать в конце метода.

for (int i = 0; i < waitingEmployees.size(); i++) {
    waitingEmployees.set(i, null);
    System.out.println(waitingEmployees.get(i));
}

Зачем тебе здесь вывод, очевидно же что get(i) вернет null. Ну и если это у тебя какой то из подвидов List интерфейса, там есть метод clear() Но в целом, что бы сказать, как это написать правильно было бы не плохо иметь чуть больше информации об условии.

W18

Уровень 19

18 октября 2022, 14:45

Спасибо Вам огромное!

W18

Уровень 19

18 октября 2022, 14:50

Условие В классе Solution есть список сотрудников waitingEmployees, которые ждут зарплату, и список сотрудников alreadyGotSalaryEmployees, которые её уже получили. Необходимо реализовать метод paySalary(String), который в качестве аргумента принимает имя сотрудника, желающего получить зарплату. Метод должен проверить наличие сотрудника в списке waitingEmployees, и если он не получал зарплату, выдать её: Если параметр метода paySalary(String) равняется null, ничего не нужно делать. Добавить имя сотрудника в список alreadyGotSalaryEmployees. В списке waitingEmployees заменить имя сотрудника на null. Requirements: 1. Метод paySalary(String) должен добавлять в список alreadyGotSalaryEmployees имя сотрудника, если он есть в списке waitingEmployees. 2. Метод paySalary(String) должен заменять имя сотрудника на значение null, если он есть в списке waitingEmployees. 3. Метод paySalary(String) не должен изменять списки waitingEmployees и alreadyGotSalaryEmployees, если переданного имени нет в списке waitingEmployees. 4. Если в метод paySalary(String) передается null, метод не должен добавлять это значение в waitingEmployees или alreadyGotSalaryEmployees.

Денис Enterprise Java Developer

18 октября 2022, 14:55

Полазил по сайту, нашел вопрос с условием задачи. С виду достаточно будет вот такого решения:

public static void paySalary(String name) {
    if (name != null && waitingEmployees.contains(name)) {
        alreadyGotSalaryEmployees.add(name);
        waitingEmployees.remove(name);
    }
}

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

waitingEmployees.set(waitingEmployees.indexOf(name), null);