Помогите Функционал кастомных предметов

[qwerqtyk]

Не уверен что тут это является нормой, но все же)
Можно получить небольшое ревью? Первый раз пишу подобное и не очень понимаю как лучше (не бейте)

Спойлер: ф

public class WateringCan implements Listener {

    final NamespacedKey key;
    final int MAX_WATER_LEVEL = 5;

    Integer waterLevel = 0;

    public WateringCan(Survival plugin) {
        key = new NamespacedKey(plugin, "water_level");
    }

    @EventHandler
    public void playerInteractEvent(PlayerInteractEvent event) {
        ItemStack itemStack = Objects.requireNonNull(event.getItem());
        boolean rightClickBlock = (event.getAction() == Action.RIGHT_CLICK_BLOCK);

        if (rightClickBlock && isWateringCan(itemStack)) {
            Block block = event.getClickedBlock();

            if (block.getRelative(event.getBlockFace()).getType() == Material.WATER) {
                waterCanFill(itemStack);
                block.getWorld().playSound(block.getLocation(), Sound.ITEM_BUCKET_FILL, 1.0f, 1.0f);
            } else if (block.getBlockData() instanceof Farmland farmland) {
                if (waterLevel > 0 && farmland.getMoisture() > 0) return;

                farmland.setMoisture(farmland.getMaximumMoisture());
                block.setBlockData(farmland);
                waterCanReduce(itemStack);

                block.getWorld().spawnParticle(Particle.WATER_SPLASH, block.getLocation(), 1);
                block.getWorld().playSound(block.getLocation(), Sound.ITEM_BUCKET_EMPTY, 1.0f, 1.0f);
            }
        }
    }

    private void waterCanReduce(ItemStack item) {
        ItemMeta itemMeta = item.getItemMeta();
        itemMeta.getPersistentDataContainer().set(key, PersistentDataType.INTEGER, waterLevel--);
        itemMeta.setLore(Collections.singletonList("*".repeat(waterLevel)));
        item.setItemMeta(itemMeta);
    }

    private void waterCanFill(ItemStack item) {
        ItemMeta itemMeta = item.getItemMeta();
        itemMeta.getPersistentDataContainer().set(key, PersistentDataType.INTEGER, MAX_WATER_LEVEL);
        itemMeta.setLore(Collections.singletonList("*".repeat(MAX_WATER_LEVEL)));
        item.setItemMeta(itemMeta);
    }

    private boolean isWateringCan(ItemStack itemStack) {
        ItemMeta itemMeta = itemStack.getItemMeta();
        return itemMeta.getPersistentDataContainer().has(key, PersistentDataType.INTEGER);
    }
}

 

[Reality_SC]

key стоит сделать private.
MAX_WATER_LEVEL стоит сделать private static или public static.
waterLevel стоит сделать private.

ItemStack itemStack = Objects.requireNonNull(event.getItem());

Честно признаюсь, давно не писал под ведроапи, может ли быть null в этом эвенте?
Возможно, лучше сделать так:

var item = event.getItem();
if (item == null) {
   return;
}

...

С точки зрения чистоты кода я бы рекомендовал выностить все многократно повторяющиеся вызовы геттеров в один, например с world:

Не жалей кол-во строчек, они бесплатные, а вот в ширину они станут короче и быстрее осознаваемыми.

Ещё по поводу вложенных if, старайся инвертировать верхний и делать early return:

boolean isNotRightClickBlock = event.getAction() != Action.RIGHT_CLICK_BLOCK; // Кстати, скобки тут не нужны.
if (isNotRightClickBlock) {
   return;
}

ItemStack itemStack = event.getItem();
if (itemStack == null || !isWateringCan(itemStack)) {
   return;
}

Block block = event.getClickedBlock();
World world = block.getWorld();

...

"Can" у тебя используется в значении "банка", а не "может", верно?


Вообще код выглядит приятно, ты молодец.

 

[qwerqtyk]

key стоит сделать private.
MAX_WATER_LEVEL стоит сделать private static или public static.
waterLevel стоит сделать private.

ItemStack itemStack = Objects.requireNonNull(event.getItem());

Честно признаюсь, давно не писал под ведроапи, может ли быть null в этом эвенте?
Возможно, лучше сделать так:

var item = event.getItem();
if (item == null) {
   return;
}

...

С точки зрения чистоты кода я бы рекомендовал выностить все многократно повторяющиеся вызовы геттеров в один, например с world:
Посмотреть вложение 64196
Не жалей кол-во строчек, они бесплатные, а вот в ширину они станут короче и быстрее осознаваемыми.

Ещё по поводу вложенных if, старайся инвертировать верхний и делать early return:

boolean isNotRightClickBlock = event.getAction() != Action.RIGHT_CLICK_BLOCK; // Кстати, скобки тут не нужны.
if (isNotRightClickBlock) {
   return;
}

ItemStack itemStack = event.getItem();
if (itemStack == null || !isWateringCan(itemStack)) {
   return;
}

Block block = event.getClickedBlock();
World world = block.getWorld();

...

"Can" у тебя используется в значении "банка", а не "может", верно?


Вообще код выглядит приятно, ты молодец.

getItem может вернуть нулл, т.к. это пустая рука, а ide это не дает покоя
wateringCan - лейка) Хезе как еще обозвать.

Хотелось бы еще узнать на счет методов... правильно ли они вынесены? К примеру до этого, я в fill юзал playsound, но потом понял, а она может быть заполнена и командой, а в ней звук не нужен) (на счет модификатора знаю и вызов эвента кастомного прикрутить)
Возможно ли как-то сократить строки с эффектами, обновлением влажности блока, убрать getItemMeta / setItemMeta? Может что-то стоить распределить по классам?
Хотелось бы сразу все исправить, т.к. подобное будет юзаться еще очень много раз :В

 

[DimaTiunov]

key стоит сделать private.
MAX_WATER_LEVEL стоит сделать private static или public static.
waterLevel стоит сделать private.

ItemStack itemStack = Objects.requireNonNull(event.getItem());

Честно признаюсь, давно не писал под ведроапи, может ли быть null в этом эвенте?
Возможно, лучше сделать так:

var item = event.getItem();
if (item == null) {
   return;
}

...

С точки зрения чистоты кода я бы рекомендовал выностить все многократно повторяющиеся вызовы геттеров в один, например с world:
Посмотреть вложение 64196
Не жалей кол-во строчек, они бесплатные, а вот в ширину они станут короче и быстрее осознаваемыми.

Ещё по поводу вложенных if, старайся инвертировать верхний и делать early return:

boolean isNotRightClickBlock = event.getAction() != Action.RIGHT_CLICK_BLOCK; // Кстати, скобки тут не нужны.
if (isNotRightClickBlock) {
   return;
}

ItemStack itemStack = event.getItem();
if (itemStack == null || !isWateringCan(itemStack)) {
   return;
}

Block block = event.getClickedBlock();
World world = block.getWorld();

...

"Can" у тебя используется в значении "банка", а не "может", верно?


Вообще код выглядит приятно, ты молодец.

jvme делает бррр