1

Тема: Чи є тут Senior Developers та хто може зробити ревю?

Взагалі то я РНР розробник. Але деколи пишу на java
Шукаю гуру програмістів які можуть зробити код ревю і вказати на помилки.


git clone https://github.com/funivan/hangman

mvn clean package
java -classpath target/classes hangman.Main
Подякували: 0xDADA11C71

2 Востаннє редагувалося Master_Sergius (18.07.2017 12:05:20)

Re: Чи є тут Senior Developers та хто може зробити ревю?

Java може й непогана річ, але мені дуже шкода джавістів коли Я бачу ось таке:

       this.game = new Game(
            new PrintStream(out),
            new CachedWords(new RandomWords(WORDS)),
            new MistakesTrack(max,
                new SolvedTrack(
                    new SuccessRoundTrack(
                        new PuzzleLog(
                            new Round(new Scanner(in))
                        )
                    )
                )
            )
        );
    }

У java є якась тулза, що дозволяє оцінити стиль коду, може якийсь jlint (як clint, pylint)? Це може допомогти

Мій блог про ОС сімейства *nix - http://nixtravelling.blogspot.com/
Подякували: koala, leofun012

3

Re: Чи є тут Senior Developers та хто може зробити ревю?

Master_Sergius це не всі так пишуть. Це я так написав. По такому самому стилю пишу в рнр
У яві як і у інших мовах є тулзи для оцінки стилю. Але ніхто вам не скаже що не можна вкладувати 10 обєктів один в один або викликати підряд 10 new new не можна ;)

Чим цей стиль вам кращий ?

Прихований текст

 String word = WORDS[new Random().nextInt(WORDS.length)];
        boolean[] visible = new boolean[word.length()];
        int mistakes = 0;
        try (final PrintStream out = new PrintStream(this.output)) {
            final Iterator<String> scanner = new Scanner(this.input);
            boolean done = true;
            while (mistakes < this.max) {
                done = true;
                for (int i = 0; i < word.length(); ++i) {
                    if (!visible[i]) {
                        done = false;
                    }
                }
                if (done) {
                    break;
                }
                out.print("Guess a letter: ");
                char chr = scanner.next().charAt(0);
                boolean hit = false;
                for (int i = 0; i < word.length(); ++i) {
                    if (word.charAt(i) == chr && !visible[i]) {
                        visible[i] = true;
                        hit = true;
                    }
                }
                if (hit) {
                    out.print("Hit!\n");
                } else {
                    out.printf(
                        "Missed, mistake #%d out of %d\n",
                        mistakes + 1, this.max
                    );
                    ++mistakes;
                }
                out.append("The word: ");
                for (int i = 0; i < word.length(); ++i) {
                    if (visible[i]) {
                        out.print(word.charAt(i));
                    } else {
                        out.print("?");
                    }
                }
                out.append("\n\n");
            }
            if (done) {
                out.print("You won!\n");
            } else {
                out.print("You lost.\n");
            }
        }

4

Re: Чи є тут Senior Developers та хто може зробити ревю?

Ну, оце варто б розбити на кілька функцій. Так, краще не стало :)

Мій блог про ОС сімейства *nix - http://nixtravelling.blogspot.com/

5

Re: Чи є тут Senior Developers та хто може зробити ревю?

Master_Sergius розібємо, але розширювати буде не можливо. ;)
Наприклад ось вам ситуація.
До цього процедурного коду треба додати такі алгоритми:
1. Показувати прогрес бар у вигляді `====>-----` тобто якщо вгадали 50%  слова показуємо пять символів = потім пять символів -
2. Якщо у юзера залишилась 1на спроба і він відгадав менше 40% - відкриваємо всі голосні

6

Re: Чи є тут Senior Developers та хто може зробити ревю?

А чому прінтами? Впевнений, що й Java вміє працювати з бібліотекою curses, може щось типу такого - https://sourceforge.net/projects/javacurses/

Мій блог про ОС сімейства *nix - http://nixtravelling.blogspot.com/

7

Re: Чи є тут Senior Developers та хто може зробити ревю?

Master_Sergius якщо я вас правильно зрозумів:
Це консольна гра. Але нам треба її розширювати. Тут основна ідея в архітектурі.

8

Re: Чи є тут Senior Developers та хто може зробити ревю?

Я явно не Java Senior, але пара питань виникла.
1. Round у грі тільки один, і передається параметром у Game? Нащо він потрібен і чим відрізняється від Game? Чому б його не створювати в конструкторі Game? Що дає додатковий рівень абстракції тут?
2. В Game.java do-while виглядав би природніше.
3. RandomWords.get кидає виняток, якщо слово (випадкове!) пусте. Може, поставити перевірку в конструктор? Бо так можна суттєві помилки пропустити на тестуванні.
4. Часта конструкція puzzle = щось_там(puzzle) (наприклад, у Round, а в Game розтягнута на два рядки). Може, варто зробити це методами puzzle, щоб не створювати і не присвоювати зайвий раз?

Подякували: funivan, leofun012

9

Re: Чи є тут Senior Developers та хто може зробити ревю?

1. Згідний фактично можна занести код game у Main.java і викинути клас Game
2. codestyle ;)
3. Це спеціально так зроблено. Раніше ішов тим самим шляхом - помилки в конструкторі. Але зараз відказався від цієї технології. Ми можемо клас ініціалізувати вище, але не факт що весь функціонал ми будемо використовувати. Відповідно не завжди треба ця штука. Тут тільки тести писати.
4. Можливо покажете прототип методу. На різних стадіях раунду нам треба з puzzle зробити різну штуку. Взяти кількість відкритих букв, відкрити букви, та інше.  Покищо не знаю як це оптимізувати.

10

Re: Чи є тут Senior Developers та хто може зробити ревю?

2. А можете гайд на github викласти?
3. Ще раз: тести можуть не покрити ситуацію. Наприклад, у вас список з 1e5 слів, одне з яких пусте - скільки треба прогнати тестів, щоб побачити проблему?
4. Давайте подивимося...

ResultInterface result = round.handle(this.output, puzzle);
puzzle = result.puzzle();

Це, гадаю, має перетворитися у щось типу

ResultInterface result = puzzle.try_open( ... );

Для цього треба буде перенести код з Round до Puzzle - що добре узгоджується з зайвістю Round. Раунд - це лише один прохід основного циклу гри, йому не потрібен окремий клас.

11

Re: Чи є тут Senior Developers та хто може зробити ревю?

2. Гайд простий https://github.com/funivan/hangman/blob … /README.md
mvn clean package
java -classpath target/classes hangman.Main
дальше відгадуєте слова ;)
3. Якщо у анс буде 1e5 слів тоді перебирати всі у конструкторі і проводити їхню валідацію буде напряжно. тут є ще один профіт у тому що ми не кидаємо exception в конструкторі.
Ми можемо написати враппер SafeRound реалізувати RoundInterface і там робити try catch якщо щось зламається ми виводимо помилку але не буде помилки при конструюванні обєктів. Тут дуже багато нюансів і у кожного свій стиль написання. Останнім часом стараюсь юзати цей підхід http://www.yegor256.com/2015/05/07/ctor … -free.html

4. Тут проблеми з назвами мабуть. Round Interface- це набір дій які мають виконуватись. У нас є наприклад клас SuccessRoundTrack
вся його задача це вивести меседж коли ми успішно вгадали. Сама пузля нічого не знає про try_open вона не займається цим.
round.handle не означає що ми зараз відкриємо букву. Один з класів - наприклад ProgressPanel буде показувати нам прогрес бар.   
Тому я в пузлю не закидував try_open

12

Re: Чи є тут Senior Developers та хто може зробити ревю?

2. Не бачу там нічого про do-while.
3. Ну так, саме в конструктор перевірку пхати не варто - там має бути виклик чогось на кшталт validate(), а вже цей validate має перевіряти. Ну, або ж додати змінну validated і викликати перевірку при першому використанні.
4. Зрештою, це ваш код. Якщо вам так зручно - то які у мене можуть бути претензії?  *PARDON*

Подякували: LoganRoss1

13

Re: Чи є тут Senior Developers та хто може зробити ревю?

koala написав:

Якщо вам так зручно - то які у мене можуть бути претензії?  *PARDON*

++

І на все у Вас своя думка, що не запропонуй ;)
Нащо тоді нас питати?

14

Re: Чи є тут Senior Developers та хто може зробити ревю?

2. Я тепер зрозумів про що ідеться мова. Стиль коду у кожної команди і у кожного програміста свій.
Попробував переписати і вийшло класно:

ResultInterface result;
do {
  result = round.handle(this.output, puzzle);
  puzzle = result.puzzle();
} while (result.next());

3. Якщо у конструкторі викликати validate у нас нічого не зміниться в плані логіки. При ініціалізації обєкта будемо обходити масив із багатьма елементами. Другий варіант із прапорцем validate можливий.

4. Можливо я щось не те сказав, але я тільки за вашу критику.
Постарався обгрунтувати свою позицію у деяких питаннях і написати чому саме я так зробив. Претензій у мене до вас немає, я навпаки вдячний за увагу і за ревю ;)

Подякували: koala1

15

Re: Чи є тут Senior Developers та хто може зробити ревю?

Мої уваги:

1) Пакети варто називати lowercase (Java Naming Convention):
"Package names are written in all lower case to avoid conflict with the names of classes or interfaces."
https://docs.oracle.com/javase/tutorial … gpkgs.html

2) Чому в мейні не можна було просто new Game(...).exec()? KISS

3) Не варто створювати інтерфейси для класів, які в 100% будуть мати лише 1 імплементацію. Це не дає ніякого профіту лише додає код. Це як interface HumanInterface -> class Human extends HumanInterface? KISS

4) Не використовуйте "Interface", "I", "A" і інше
https://stackoverflow.com/a/2814831

5) Зберігайте назву інтерфейсу/класу що імплементується/розширюється в назві за можливістю, легше орієнтуватися. Тобто побачивши MistakesTrack Ви радше будете очікувати implements/extends Track ніж Round.

6) Не ініціялізуйте об'єкти так, як це зроблено в мейні. Можна ж аргументи сконструювати в окремих лінях? Читати неможливо.

7) Не використовуйте генеричні назви для методів/класів, де їх наявність не визначає на 100% їх інтенцію. Тобто RoundInterface#handle мені особисто нічого не каже і чому він хендлить якийсь інший пазл з стрімом.

Нарешті 0) Порівняйте кількість Вашого коду і в оригіналі (<100LOC vs >800LOC). Вийшов overengineering. KISS.

А взагалі, код як код. Є речі які я б зробив інакше, але радше через суб'єктивні причини.

Подякували: koala, 0xDADA11C7, ostap34PHP, funivan, leofun015

16

Re: Чи є тут Senior Developers та хто може зробити ревю?

iovchynnikov дякую ;)
1. ок ;)
2. Так я уже вище погодився що можна обійтись і без game
3. Я частково згідний але частково ні)) Що значить 100% будуть мати одну реалізацію? Якщо не враховувати GameInterface - назвати приклад інтерфейсу який буде 100% реалізовано тільки один раз?
4. Наскільки я розумію MessageInterface мав бути Message а як тоді назвати клас Message ?
5. Тут згідний 100%
6. Типу оголошуємо змінну і змінній присвоюємо інстанс обєкту? Це діло привички, мені раніше також було важко це зробити. Але зараз дуже зручно. Але вашу точку зору я розумію, спочатку це може взривати мозок.
7.  як назвати метод handle ? Типу вступауй у роль твоя черга) Мені здається як його не назви ніяк добре не буде)) Якщо можете наведіть ще приклади де є погані назви методів.

Нарешті 0) ну тут 100% але ви розумієте що LOC це взагалі не показник. Візьміть метрику NPath ;)
Як я писав вище основна ідея це написати код який легко розширити і змінити. І ви прекрасно розумієте що у оригінальному варіанті ми ще можемо додати якісь там 2-4 умови і доповнювати алгоритм. Але настане момент коли ми не зможемо безболісно вклинюватись в процес.
Ще одна перевага у 800LOC це тести. Попробуйте напишіть тести для старої версії. У вас буде здоровий тест який завжди запускаєм main і перевіряє що він виводить)

Дуже вам вдячний за ревю і за посилання ;) Над деякими штуками дійсно варто задуматись.

17

Re: Чи є тут Senior Developers та хто може зробити ревю?

Треба було мені назвати не RoundInterface a JudgeInterface і метод не handle а operate Тоді мабуть менше б було плутанитит
У роль вступає 10 суддів, по черзі вони щось вирішують. Хтось приймає рішення про те чи можна відгадувати чи ні, хтось показує рахунок а хтось питає у клієнта букву. Що скажете ;)

18

Re: Чи є тут Senior Developers та хто може зробити ревю?

funivan написав:

iovchynnikov дякую ;)
1. ок ;)
2. Так я уже вище погодився що можна обійтись і без game
3. Я частково згідний але частково ні)) Що значить 100% будуть мати одну реалізацію? Якщо не враховувати GameInterface - назвати приклад інтерфейсу який буде 100% реалізовано тільки один раз?
4. Наскільки я розумію MessageInterface мав бути Message а як тоді назвати клас Message ?
5. Тут згідний 100%
6. Типу оголошуємо змінну і змінній присвоюємо інстанс обєкту? Це діло привички, мені раніше також було важко це зробити. Але зараз дуже зручно. Але вашу точку зору я розумію, спочатку це може взривати мозок.
7.  як назвати метод handle ? Типу вступауй у роль твоя черга) Мені здається як його не назви ніяк добре не буде)) Якщо можете наведіть ще приклади де є погані назви методів.

Нарешті 0) ну тут 100% але ви розумієте що LOC це взагалі не показник. Візьміть метрику NPath ;)
Як я писав вище основна ідея це написати код який легко розширити і змінити. І ви прекрасно розумієте що у оригінальному варіанті ми ще можемо додати якісь там 2-4 умови і доповнювати алгоритм. Але настане момент коли ми не зможемо безболісно вклинюватись в процес.
Ще одна перевага у 800LOC це тести. Попробуйте напишіть тести для старої версії. У вас буде здоровий тест який завжди запускаєм main і перевіряє що він виводить)

Дуже вам вдячний за ревю і за посилання ;) Над деякими штуками дійсно варто задуматись.

3) це означає що у інтерфейса завжди буде лише один клас, що його імплементує. Ситуація більш відома як Interface - InterfaceImpl. Який сенс у 'пазлІнтерфейс' якщо єдина очікувана імплементація це 'Пазл'? Для мене це те ж саме якби в jdk було StringInterface & String implements StringInterface.

4) Інтерфейс там непотрібний.

class Message {
    private final String msg;
    Message(String msg) {
        this.msg=msg;
    }
}
class EmptyMessage extends Message {
    EmptyMessage() {
        super("");
    }
}
// ....

6) Так. Окремі лінії також легше дебажити.
7) використовуєте слова/вирази з того домену і контексту і якому Ви працюєте/знаходиться код. Як я й сказав, я не дуже зрозумів що воно робить тому кращого варіянту радше не запропоную. Якийсь "puzzleToStream" був би краще (якщо назва відповідає ідеї метода) ніж хендл, який настільки генеричний що може робити трохи менше ніж будь-що. Ідею зрозуміли.

Я розумію що це проект радше академічний, просто майте на увазі, що коли програміст каже "настане момент коли ми не зможемо безболісно вклинюватись в процес" в більшості випадків це означає "я просто хотів зробити код іліґантньим", а той "момент" ніколи не настане. Це так радше зі свого особистого досвіду :))

Подякували: leofun011

19

Re: Чи є тут Senior Developers та хто може зробити ревю?

3. Ідея зрозуміла. Але от наприклад мені треба буде статистика по буквах які народ найчастіше вгадує. Я зроблю дуже просто. Напишу клас LoggablePuzzle

Прихований текст

public class LoggablePuzzle implements PuzzleInterface {

    public LoggablePuzzle(PuzzleInterface original, LogInterface log) {
        this.original = original;
    }

    @Override
    public boolean contains(Character letter) {
        return this.original.contains(letter);
    }

    @Override
    public PuzzleInterface open(Character letter) {
        PuzzleInterface result = this.original.open(letter);
        this.logger.logLetter(letter);
        return result;
    }

}


Я згідний що типу це надуманий приклад. Але це все також з особистого досвіду. Я мабуть ще створю подібну тему в РНР і покажу як було і як стало уже на реальному проекті і з реальними задачами.

20

Re: Чи є тут Senior Developers та хто може зробити ревю?

funivan написав:

3. Ідея зрозуміла. Але от наприклад мені треба буде статистика по буквах які народ найчастіше вгадує. Я зроблю дуже просто. Напишу клас LoggablePuzzle

Прихований текст

public class LoggablePuzzle implements PuzzleInterface {

    public LoggablePuzzle(PuzzleInterface original, LogInterface log) {
        this.original = original;
    }

    @Override
    public boolean contains(Character letter) {
        return this.original.contains(letter);
    }

    @Override
    public PuzzleInterface open(Character letter) {
        PuzzleInterface result = this.original.open(letter);
        this.logger.logLetter(letter);
        return result;
    }

}


Я згідний що типу це надуманий приклад. Але це все також з особистого досвіду. Я мабуть ще створю подібну тему в РНР і покажу як було і як стало уже на реальному проекті і з реальними задачами.

Чесно кажучи, не дуже розумію в чому сенс такого декоратора, пазла в пазлі. Замініть композицію успадкуванням (LoggablePuzzle extends Puzzle) і можна зробити те ж саме.

Подякували: leofun011