Java - мой код явно противоречит общим парадигмам OOD, но не уверен, как его улучшить

Ответ 1

Здесь простой способ сделать это, не обязательно лучший, но он отвечает следующим требованиям:

• Уметь динамически получать специализированный тип из коллекции Unit
• Уметь добавлять дополнительные типы Unit позже, без необходимости добавлять кучу методов обработчика.

В решении используется класс 'template' для выполнения сопоставления:

@SuppressWarnings("unchecked")
public <T extends Unit> T getSpecializedUnitType(Class<T> unitTypeClass, String uniqueID) {
    Unit potentialTroop = units.get(uniqueID);
    if(potentialTroop == null) return null;

    return potentialTroop.getClass().equals(unitTypeClass) ?
        (T) potentialTroop : null;
}

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

Ответ 2

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

Тогда у вас будет больше гибкости в том, как вы храните объекты. В вашем случае вы можете иметь 3 карты внутри; один из которых содержит все подразделения, один для войск и один только для транспортных средств. Данный блок будет храниться на двух картах, поэтому вам нужно будет синхронизировать добавление и удаление единиц для обеспечения целостности между различными картами.

Ответ 3

Почему бы не реализовать только один метод get, который возвращает Unit? Я считаю, что если у двух классов есть очень конкретные, разные методы, то у них нет много общего, не так ли? Тот факт, что вы хотите, чтобы они были подклассами Unit, заставляет меня думать, что их сходство выходит за рамки факта "они - объекты с позицией на карте". В этом случае "неправильная" часть будет давать методам разные имена и вызывать их отдельно. Способ сделать вещи лучше:

• для возврата только единицы из хеш-таблицы
• как уже было предложено, при необходимости использовать комплексные методы, такие как behave(), move(), которые могут быть общими для обоих классов и могут поэтому следует называть без кастинга (хотя они и делают разные вещи). Используйте созданную вами иерархию! Эти методы в конечном итоге вызовут небольшие, модульные методы, которые вы создали ранее (теперь это станет private).
• Вам не всегда нужно знать тип Unit, не так ли? Только когда это необходимо (не разрешимо с помощью упомянутых выше методов), делегируйте такую ​​работу тому, что вы определили как "вызывающий", который, как я считаю, не является единой фиксированной сущностью. Другими словами, выполните проверку типа только тогда, когда вам нужно решить, стреляет ли ваша единица с помощью винтовки или нет. Если вы хотите выполнять общие задачи, нет необходимости предвидеть такое различие.

Ответ 4

Этот метод getVehicle() и getTroops() почти похож на цепочку операторов if (elem instanceof X), кроме исключений. Я ненавижу такую ​​цепочку, но в этом случае я предпочел бы это и оставил бы хэш-таблицу в одиночку.

НО, если конкретные методы, которые вам нужно вызвать, можно рассматривать как "специфическое поведение" Vehicle или Troop, тогда вы должны рассмотреть возможность их добавления абстрактным методом в классе Unit и переопределить его в каждом классе. Если это не так, просьба предоставить дополнительную информацию о том, что вам нужно сделать с вашим подразделением, чтобы мы могли найти хорошее обобщение.

Ответ 5

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

Здесь вероятность того, что я думаю, лучше, хотя некоторые утверждают, что она по-прежнему не совсем хороша из-за методов, которые вызывают UnsupportedOperationException. Я использовал Groovy, чтобы быть более кратким, но достаточно близко к Java, чтобы вы поняли эту идею. Посмотрите, соответствует ли это вашим потребностям:

abstract class Unit {
    enum UnitType { TROOP, VEHICLE }
    abstract UnitType getType()
    TroopAbilities getTroopAbilities() {
        throw new UnsupportedOperationException('not a Troop') 
    }
    VehicleAbilities getVehicleAbilities() { 
        throw new UnsupportedOperationException('not a Vehicle') 
    }
}

interface TroopAbilities {
    void doTroopThing()
}

interface VehicleAbilities {
    void doVehicleThing()
}

class Troop extends Unit implements TroopAbilities {
    void doTroopThing() { println 'something troopy' }
    UnitType getType() { UnitType.TROOP }
    TroopAbilities getTroopAbilities() { this }
}

class Vehicle extends Unit implements VehicleAbilities {
    void doVehicleThing() { println 'something vehicle-ish' }
    UnitType getType() { UnitType.VEHICLE }
    VehicleAbilities getVehicleAbilities() { this }
}

List<Unit> units = [new Troop(), new Vehicle(), new Troop()]
for (Unit unit : units) {
    switch (unit.getType()) {
        case Unit.UnitType.TROOP:
            unit.getTroopAbilities().doTroopThing()
            break;
        case Unit.UnitType.VEHICLE:
            unit.getVehicleAbilities().doVehicleThing()
            break;
        default:
            throw new IllegalStateException(
                "New unit type that not accounted for: " + unit.getType())
    }
}

Кроме того, ваша жизнь будет проще, если вы сделаете чистый разрыв между интерфейсом и реализацией, так что Unit действительно должен быть более похож:

interface Unit {
    enum UnitType { TROOP, VEHICLE }
    UnitType getType()
    TroopAbilities getTroopAbilities()
    VehicleAbilities getVehicleAbilities()
}

abstract class AbstractUnit implements Unit {
    TroopAbilities getTroopAbilities() {
        throw new UnsupportedOperationException('not a Troop')
    }
    VehicleAbilities getVehicleAbilities() {
        throw new UnsupportedOperationException('not a Vehicle')
    }
}

Тогда ваши конкретные типы единиц расширят AbstractUnit, а затем вы правильно используете наследование для повторного использования кода и полиморфизма, чтобы каждый подкласс мог реагировать на сообщения по-своему. Единственная серая область - это методы get * Abilties(), но я не могу придумать хороший способ в настоящее время.

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

interface Unit {
    abstract String getType()
    Troop asTroop()
    Vehicle asVehicle()
}

abstract class AbstractUnit implements Unit {
    Troop asTroop() {
        throw new UnsupportedOperationException('not a Troop')
    }
    Vehicle asVehicle() {
        throw new UnsupportedOperationException('not a Vehicle')
    }
}

class Troop extends AbstractUnit {
    void doTroopThing() { println 'something troopy' }
    String getType() { "troop" }
    Troop asTroop() { this }
}

class Vehicle extends AbstractUnit {
    void doVehicleThing() { println 'something vehicle-ish' }
    String getType() { "vehicle" }
    Vehicle asVehicle() { this }
}

List<Unit> units = [new Troop(), new Vehicle(), new Troop()]
for (Unit unit : units) {
    switch (unit.getType()) {
        case "troop":
            unit.asTroop().doTroopThing()
            break;
        case "vehicle":
            unit.asVehicle().doVehicleThing()
            break;
        default:
            throw new IllegalStateException(
                "New unit type that not accounted for: " + unit.getType())
    }
}

Ответ 6

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

public <T extends Unit> T getSpecializedUnitType(Class<T> unitClass, String id) {
    Unit unit = units.get(id);
    return unitClass.cast(unit);
}

Нет подавленных предупреждений, если unit is null, возвращается null, если нет, то он отображается в нужный тип. Если тип ошибочен, вызывается ClassCastException.