JavaRush/Помощь по Java задачам/Нужно код ревью опытных коллег. Обоснованность применения...

29 уровень

01.08.2020
490просмотров
8комментариев

Нужно код ревью опытных коллег. Обоснованность применения паттерна. Коректность JavaDoc

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

Обсуждается

Доброго дня! Два вопроса к опытным коллегам: 1. Насколько корректно применение шаблона проектирования "Стратегия" для решения CrUD-задач (не конкретной данной task17.task1711 а подобного класса задач в принципе) и, самое главное, насколько адекватно он в данном случае реализован? 2. Как принято комментировать подобные решения? Если я достаточно очевидно реализую паттерн "Стратегия", нужно ли комментировать что в данной конкретной имплементации содержит контекст и что делает екзекьютор конкретной стратегии? Поскольку, я не могу прикрепить к вопросу код решения моей задачи, так как она уже решена, пришлось состряпать портянку из вложенных классов, а т.к. публиковать правильные решения запрещено, ловите валидную с точки зрения компилятора реализацию с парой неочевидных багов, которые гарантированно не пропустит валидатор:

package com.javarush.task.task17.task1711;

import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.util.*;

/**
 * Strategy interface for concrete strategies implementations
 */
interface Strategy {
    /**
     * Strategy executor for concrete strategies implementations
     */
    void execute();
}

/**
 * Context class holding concrete strategy instance
 */
class Context {
    private Strategy strategy;

    /**
     * Class constructor specifying concrete strategy to execute.
     *
     * @param strategy concrete strategy instance
     */
    public Context(Strategy strategy) {
        this.strategy = strategy;
    }

    /**
     * Method launching concrete strategy execution
     */
    public void execute() {
        strategy.execute();
    }
}

/**
 * Common strategy template
 */
class TemplateStrategy implements Strategy {
    protected String[] params;
    protected List<Person> personList;

    /**
     * Empty Strategy executor
     */
    @Override
    public void execute() {
        //do nothing
    }

    /**
     * Template strategy constructor specifying operation parameters and person list instance to operate.
     *
     * @param params
     * @param personList
     */
    public TemplateStrategy(String[] params, List<Person> personList) {
        this.params = params;
        this.personList = personList;
    }
}

/**
 * Concrete strategy for adding persons to list.
 */
class CreateStrategy extends TemplateStrategy implements Strategy {
    public CreateStrategy(String[] params, List<Person> personList) {
        super(params, personList);
    }

    /**
     * Create strategy executor.
     * Adds all people with assigned parameters to people list end,
     * prints id (index) in the console in corresponding order
     */
    @Override
    public void execute() {
        synchronized (personList) {
            for (int i = 0; i < params.length; i += 3) {
                try {
                    Date date = new SimpleDateFormat("dd/MM/yyyy").parse(params[i + 2]);
                    switch (params[i + 1].toLowerCase()) {
                        case "м":
                            personList.add(Person.createMale(params[i], date));
                            break;
                        case "ж":
                            personList.add(Person.createFemale(params[i], date));
                            break;
                    }
                } catch (ParseException e) {
                    e.printStackTrace();
                }
                System.out.println(personList.size() - 1);
            }
        }
    }
}

/**
 * Concrete strategy for updating person data in the list.
 */
class UpdateStrategy extends TemplateStrategy implements Strategy {
    public UpdateStrategy(String[] params, List<Person> personList) {
        super(params, personList);
    }

    /**
     * Update strategy executor.
     * Updates corresponding data of person with defined id
     */
    @Override
    public void execute() {
        synchronized (personList) {
            for (int i = 0; i < params.length; i += 4) {
                int idx = Integer.parseInt(params[i]);
                Person person = personList.get(idx);

                person.setName(params[i + 1]);

                Sex sex;
                switch (params[i + 2].toLowerCase()) {
                    case "м":
                        sex = MALE;
                        break;
                    case "ж":
                        sex = FEMALE;
                        break;
                    default:
                        sex = null;
                        break;
                }
                person.setSex(sex);

                try {
                    person.setBirthDate(new SimpleDateFormat("dd/MM/yyyy").parse(params[i + 3]));
                } catch (ParseException e) {
                    e.printStackTrace();
                }
            }
        }
    }
}

/**
 * Concrete strategy for logical deleting persons data.
 */
class DeleteStrategy extends TemplateStrategy implements Strategy {
    public DeleteStrategy(String[] params, List<Person> personList) {
        super(params, personList);
    }

    /**
     * Delete strategy executor.
     * Provides logical deleting person with defined id,
     * replaces all its data with null
     */
    @Override
    public void execute() {
        synchronized (personList) {
            for (int i = 0; i < params.length; i++) {
                Person person = personList.get(i);
                person.setName(null);
                person.setSex(null);
                person.setBirthDate(null);
            }
        }
    }
}

/**
 * Concrete strategy for providing information about person with defined id.
 */
class InfoStrategy extends TemplateStrategy implements Strategy {
    public InfoStrategy(String[] params, List<Person> personList) {
        super(params, personList);
    }

    /**
     * Info strategy executor.
     * prints in the console following data about person:
     * name sex and birth date (format: 15-Apr-1990),
     * id is corresponding to the index of element in list
     */
    @Override
    public void execute() {
        synchronized (personList) {
            for (int i = 0; i < params.length; i++) {
                Person person = personList.get(i);
                String sex;
                switch (person.getSex()) {
                    case MALE:
                        sex = "м";
                        break;
                    case FEMALE:
                        sex = "ж";
                        break;
                    default:
                        sex = null;
                        break;
                }
                System.out.println(
                        person.getName() + " " + sex + " " +
                                new SimpleDateFormat("dd-MMM-yyyy", Locale.ENGLISH).format(person.getBirthDate())
                );
            }
        }
    }
}

/**
 * Concrete strategy for default behaviour.
 */
class DefaultStrategy implements Strategy {

    /**
     * Empty Strategy executor
     */
    @Override
    public void execute() {
        //do nothing
    }
}

public class Solution {
    public static volatile List<Person> allPeople = new ArrayList<Person>();

    static {
        allPeople.add(Person.createMale("Иванов Иван", new Date())); //сегодня родился id=0
        allPeople.add(Person.createMale("Петров Петр", new Date())); //сегодня родился id=1
    }

    public static void main(String[] args) {
        if (Objects.isNull(args) || 0 == args.length) {
            return;
        }

        Context context;
        String[] params = new String[args.length - 1];
        for (int i = 0; i < params.length; i++) {
            params[i] = args[i + 1];
        }

        switch (args[0]) {
            case "-c":
                //unnecessary syncronized block, just for passing validator
                synchronized (allPeople) {
                    context = new Context(new CreateStrategy(params, allPeople));
                }
                break;
            case "-u":
                //unnecessary syncronized block, just for passing validator
                synchronized (allPeople) {
                    context = new Context(new UpdateStrategy(params, allPeople));
                }
                break;
            case "-d":
                //unnecessary syncronized block, just for passing validator
                synchronized (allPeople) {
                    context = new Context(new DeleteStrategy(params, allPeople));
                }
                break;
            case "-i":
                //unnecessary syncronized block, just for passing validator
                synchronized (allPeople) {
                    context = new Context(new InfoStrategy(params, allPeople));
                }
                break;
            default:
                //unnecessary syncronized block, just for passing validator
                synchronized (allPeople) {
                    context = new Context(new DefaultStrategy());
                }
                break;
        }
        //necessary syncronized blocks are in concrete strategy implementations
        context.execute();
    }
}

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

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

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

JustinianJudge в Mega City One

Master

5 августа 2020, 00:41

Проще всего отталкиваться от конкретного кода конечно, для данной задачи пары классов - за глаза хватит. Для CRUD подобного подхода ни разу не видел, 4 базовых операции объединенные общей реализацией, смысл их дробить в простых кейсах, а эта задача, это простой кейс. Вот когда операций не 4, а 24, мы не знаем сколько их всего будет, и каждая из них со своими приколами, вот тогда Стратегия да, то что доктор прописал. По джава докам, та вроде похоже что-то, главное не пиши так каждый класс, который пишешь :) Потренируйся немного документировать и оставь, а то мало ли, вдруг ты решишь весь код такой писать :) Ну там точечку в конце предложений не забывай, параметры не описаны, просто открывай стандартную библиотеку джавы и смотри там по образу и подобию.

* prints name sex and birth date (format: 15-Apr-1990),

хороший пример, в чем большой минус джавадоков - их нужно поддерживать. Один написал код. Второму прилетела срочная таска, надо поменять формат, хряц бац, поменяли код, джава док остался. Третий читает код - в джава доке одно, в коде другое, и что он должен думать? Что кто-то проморгал джава док исправить, или в коде бага и формат попутали? Все строки с //, типа

//unnecessary syncronized block, just for passing validator

или

//necessary syncronized blocks are in concrete strategy implementations

или

//do nothing

это мусор, ему не место в коде. i += 3 в принципе покатит, но не увлекайся сокращенными формами. А так, хорошо что экспериментируешь, продолжай. Пиши код, смотри, думай, пробуй так или иначе. Думай, стало лучше или хуже.

Arseniy Zhuck

Уровень 30

1 августа 2020, 13:27

В класс template strategy некорректно указывать хоть какие-то поля, этот класс по сути должен лишь реализовывать execute, поля должны быть указаны в context и в этом случае вы передаете его в качестве аргумента. Опять же возникнет вопрос, а как быть с private полями базового класса, здесь как раз и нужны inner class'ы как таковые. Ну или танцы с бубном. Плюс refactoring.guru Вам в помощь, там достаточно прозрачно описана стратегия

Андрей

Уровень 29

Expert

1 августа 2020, 14:02

TemplateStrategy я сделал чтобы не писать в конструкторе каждой конкретной стратегии

this.params = params;
this.personList = personList;

Хотя, необходимость вызова конструктора родительского класса и нивелирует эту "оптимизацию"

super(params, personList);

Тем не менее, я получаю возможность реализации какого-то общего поведения всех стратегий без копипасты кода. В класс template strategy некорректно указывать хоть какие-то поля, этот класс по сути должен лишь реализовывать execute, поля должны быть указаны в context и Это догма? Возможно, стоит реализовать Dependency Injection и каким-то образом передавать в стратегию ссылку на контекст? Плюс refactoring.guru Вам в помощь, там достаточно прозрачно описана стратегия Я знаком и с refactoring.guru и с Christopher Okhravi и с курсом JetBrains Academy и трудом GoF - вопрос по данной конкретной реализации

Arseniy Zhuck

Уровень 30

1 августа 2020, 14:16

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

Андрей

Уровень 29

Expert

4 августа 2020, 17:18

> именно так и логично делать, банально передавая ссылку в качестве параметра в метод в метод execute? Хмм, прикольно, надо попробовать. Но пока я родил ещё один костыль > можно хранить как поле объект контекста, и передавать его в конструктор Я не могу просто передать контекст в конструктор стратегии потому что я экземпляр стратегии подаю в конструктор контекста, т.е. при создании стратегии у меня нет ссылки на контекст, он ещё не существует

Arseniy Zhuck

Уровень 30

4 августа 2020, 20:22

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

Arseniy Zhuck

Уровень 30

4 августа 2020, 20:25

А вообще, передавать контекст параметром в принципе уберет необходимость задумываться о сеттере контекста для стратегии или о конструкторе, тогда и абстрактный класс не нужен.

Андрей

Уровень 29

Expert

5 августа 2020, 02:31

Ок, попробую. Спасибо!