1.14.4 Item not deleted/gets cloned?

Discussion in 'Spigot Plugin Development' started by enesmelda, Mar 23, 2020.

  1. Ok so,to describe my Problem better,here is a video link:



    If link does not work,link is also in the pastebin.
    Pastebin for Code and Link:
    https://pastebin.com/VSUaipnJ

    Ok so,as you can see,the item gets "cloned".Well,not realy,its probably just not getting deleted.
    I tried everything,but it either just gave me just one unchanged item,or nothing.

    My Goal is to change the item Lore on click.
    The 1st item gets deleted as planned.But I did not manage to change the item Lore of the clicked item directly,so I added a new item to the inventory,with the changed Lore. The Problem is: The unchanged Item with lore is still there.
    Debugs go through.

    Can anyone help?
     
  2. drives_a_ford

    Moderator

    There are so many issues with your code.
    I would suggest taking a look at Beginner Programming Mistakes and Why You're Making Them.

    For one, this is what I'd call spaghetti code. You're trying to cram everything in the same place making it damn near impossible to follow what you're doing. You should section parts of your code into methods that have perform a single task. For instance, a method that checks if this click is relevant for you; a method that changes an item appropriately; a method that sets the item the player is holding in the cursor.

    Your plugin field shouldn't really be package private but rather private.
    On a side note, Main is a terrible name for a plugin's class. A classname should be descriptive.

    Every time there's an inventory click, you're creating an ItemStack i. It looks like it always remains the same so you should create it once in the constructor and save it in a field. You can then use it when needed.
    The same is true for i2.

    You shouldn't use & and ChatColor#translateAlternativeColorCodes when defining string in your code. You should rather simply use the ChatColor enum constants (in your case ChatColor.GOLD). If nothing else, It improves readability.


    Instead of nesting if statements, you should be negating and returning. It reduces indenting and helps readability.
    So instead of
    Code (Java):
    if (/*condition1*/) {
        if (/*condition2*/) { // and so on
    you'd do
    Code (Java):
    if (!/*condition1*/) return;
    if (!/*condition2*/) return;
    // the rest of your conditions and/or code

    As for your issue - it looks like you're completely redefining what's happening. So you probably want to cancel the event to cancel default beahviour.
     
  3. The code is just what I used for testing. Of course I wont do spaghetti code in my actual code.
    I just made it like this because it was way faster then setting up new methods and calling them.
    So I didnt do the basic stuff,because of this being a test.
    And for the null returns,I make them like this because I like it more this way.But if you say that the null check wont work because of the way I did it,then I would change it.
    I agree on the point,that Main is not a descriptive name. But like for my case it does its job.

    Ty,I will try youre suggestion of cancelling the event and report back.
     
  4. drives_a_ford

    Moderator

    The null checks are fine. They are set up the way I described. But you do have some other if clauses that are setup differently.
     
  5. I see.
    I cancelled the event,but nothing realy changed. The old item is still on my cursor