Solved Function chaining error

Discussion in 'Plugin Development' started by Betagear, Dec 20, 2015.

Thread Status:
Not open for further replies.
  1. Offline

    Betagear

    Hi.
    I'm using ArrayLists to store locations, but when I register a lot of locations very fast, a lot of them are skipped : only the first ones are registered.
    Is anyone knowing a way to force the registration of them ?
    Btw my code is flawless.
     
  2. Offline

    adam753

    That is very unlikely, unless you've found a Java bug. Which you haven't. What's your code?
     
  3. Offline

    Betagear

    Code:
        @SuppressWarnings("deprecation")
        private List<Location> BlockRenewer(final Block start,final Block block, final BlockState Active, final BlockState Inactive, final int duration, final int Distance, final boolean area, final double wavespeed, final String waveuid, final boolean Explode, final int ExplosionForce, final Player Larbin, final boolean zone){
            List<Location> nr = new ArrayList<>(); //Creating the Arraylist : If the tests aren't sucessful it'll be empty
            final World blockworld = block.getWorld();
            final Location blocklocation = block.getLocation();
            if (zone) { //Test to see if the function is called after being pre-rendered
                main.Debug("!!! ZONE !!!");
            }
          
            if (area) { //Always true in our case
                if (!block.hasMetadata(waveuid)) { //A UUID to force the plugin to not loop between block
                    boolean colorsuccess = false; //Test to see if the block at the location corresponds with the starting block
                    if (block.getType() == Active.getType()) {
                        if (Active.getRawData() != (byte) 0 && Inactive.getRawData() != (byte) 0) {
                            if (block.getData() == Active.getRawData()) {
                                colorsuccess = true;
                            }else if (block.hasMetadata(main.mColor)) {
                                if (block.getMetadata(main.mColor).get(0).asByte() == Active.getRawData()) {
                                    colorsuccess = true;
                                }
                            }
                        } else {
                            colorsuccess = true;
                        }
                    }
                    boolean Reloadable = false;
                    if (block.getType() == main.WorldUtils.getInfo(MatInfo.Colored, blockworld) || block.getType() == main.WorldUtils.getInfo(MatInfo.LandMine, blockworld)) {
                        Reloadable = true;
                    }
                    if (Reloadable && colorsuccess && block.hasMetadata(main.mInactive) == false) { //Tests if the previous tests have been passed and the block isn't inactive. For the simplicity, let's say that it's passed
                      //All the following is purely animations
                        int truedelay = (int) (Math.round(start.getLocation().distance(blocklocation) * wavespeed)); //Calculation of the delay for the block to be reloaded.
                      
                        new BukkitRunnable() {
                          
                            @Override
                            public void run() {
                                block.setMetadata(main.mRUUID, new FixedMetadataValue(plugin, waveuid));
                                BlockReload(duration, Distance, block, Active, Inactive, waveuid);
                                if (Explode) {
                                    main.Explosions.SimulateExplosion(block.getLocation().add(0.5, 0.5, 0.5), ExplosionForce, 0.5, 1, false, Larbin, true);
                                }
                            }
                        }.runTaskLater(plugin, truedelay);
                      
                        block.setMetadata(waveuid, new FixedMetadataValue(plugin, true)); //Setting the anti-loop metadata
                      
                        new BukkitRunnable() {
                          
                            @Override
                            public void run() {
                                block.removeMetadata(waveuid, plugin);
                              
                            }
                        }.runTaskLater(plugin, 1200);
                        if (!zone) { //If zone is false, in the case of pre-rendering it
                            nr.add(blocklocation); //Adds itself in the List
                            for (Location loc : getNearBlocks(block.getLocation(), getType.PLANE)) { //Then it runs the same function all around him, and retrieves the locations
                                List<Location> tempz = BlockRenewer(start, loc.getBlock(), Active, Inactive, duration, Distance, area, wavespeed, waveuid, Explode, ExplosionForce, Larbin, zone);
                                if (!tempz.isEmpty()) { //Test if the function was successful or not, it may be not if the block tested isn't the same as the first block or the block as already been processed
                                    nr.addAll(tempz); //Adds all the values of the function into this one
                                }
                            }
                          
                            if (!main.WorldUtils.getInfo(BooleanInfo.Planear, blockworld)) { //Same thing, but into two other plans
                              
                                for (Location loc : getNearBlocks(block.getLocation(), getType.NONPLANE)) {
                                    BlockRenewer(start, loc.getBlock(), Active, Inactive, duration, Distance, area, wavespeed, waveuid, Explode, ExplosionForce, Larbin, zone);
                                    List<Location> tempz = BlockRenewer(start, loc.getBlock(), Active, Inactive, duration, Distance, area, wavespeed, waveuid, Explode, ExplosionForce, Larbin, zone);
                                    if (!tempz.isEmpty()) {
                                        nr.addAll(tempz);
                                    }
                                }
                            }
                        }
                    }
                }
            } else {
                String reloadUUID = UUID.randomUUID().toString();
                block.setMetadata(main.mRUUID, new FixedMetadataValue(plugin, reloadUUID));
                BlockReload(duration, Distance, block, Active, Inactive, reloadUUID);
            }
            return nr; //Then it return what he got.
        }
        
    Then nr is stored inside a special class, and then it tells me wich blocks were inside. For small numbers of locations, it works fine, but if there is too much it struggles.
     
    Last edited: Dec 20, 2015
  4. Offline

    adam753

    @Betagear
    Well, without studying your code for a really long time, the only thing that stands out is that you don't seem to be adding anything at all if "zone" is true. But I assume you know that.

    If I were you, I'd try to narrow down some reproduction steps, find out exactly what makes the problem happen. Is it a certain number of locations? Maybe it happens when a certain variable or condition is true? Is there any case where you do the exact same thing twice, and get different results? etc.
     
  5. Offline

    Betagear

    @adam753 "zone" is used after the locations have stored, to rerun the thing with only the stored locations, saving some ressources

    It happens after more than 50 locations stored. As I register the locations in this order (+x, +z, -x, -z), the locations are all stored in the direction +x and +z, as they are the first to come. So, it depends on the starting location.

    As you may have seen, it is maybe in the way I create the list that makes a problem.
     
  6. Offline

    mythbusterma

    @Betagear

    I mean, "flawless" is not the word I would use.

    It is an error in your code if all the Locations are not being added. It is not the fault of the ArrayList. That much we know. So now that we know the error is in your code, try debugging your code.

    You're using a lot of class-level variables where they really shouldn't be being used. This makes code extremely hard to reason about, which is probably why you're having issues figuring out where the problem is in your code. Reduce the over usage of class level variables, split that method apart, reduce the number of parameters, and format your code better. Then you might have a fighting chance of understanding your issue(s).
     
    adam753 likes this.
  7. Offline

    Betagear

    @mythbusterma Ok. All the variables are used to block the function from looping between blocks and selecting only a type of block. Somes are used in order to make an animation.
    The only part that may interest you is from the line 57 to 75.
    I'll add comments.
     
  8. Offline

    mythbusterma

    @Betagear

    Is this function recursive? If that's the case, you really need to fix this. This code is atrocious.
     
  9. Offline

    BreezerFly

    For curiousity's sake. Why is it bad to have a recursive method? Or are you just referring to this context?

    @Betagear
    Can you try refactoring your code into smaller methods/classes? May help you catch the error, or at least understand better where it goes wrong. As @mythbusterma said, makes it easier to reason about.
     
  10. Offline

    mythbusterma

    @BreezerFly

    It's not necessarily bad, but I don't see any base case here, and not only that, there's no reason for something like this to be recursive, and a recursive function should be extremely short and easy to reason about, this one is neither.

    It's also just a general rule to prefer iteration over recursion, where both are practical.
     
  11. Offline

    BreezerFly

    @mythbusterma
    Woop, agreed. Was just curious, I like using recursive functions to make code easier to reason about. :p
    And yeah, iteration is generally faster as well, right?
     
  12. Offline

    Betagear

    Here it is :
    Code:
        private List<Location> BlockRenewer(final Block start,final Block block, final BlockState Active, final BlockState Inactive, final int duration, final int Distance, final boolean area, final double wavespeed, final String waveuid, final boolean Explode, final int ExplosionForce, final Player Larbin, final boolean zone){
            List<Location> nr = new ArrayList<>();
            final World blockworld = block.getWorld();
            final Location blocklocation = block.getLocation();
            if (zone) {
                main.Debug("!!! ZONE !!!");
            }
           
            if (area) {
                if (!block.hasMetadata(waveuid)) {
                    boolean Reloadable = false;
                    if (block.getType() == main.WorldUtils.getInfo(MatInfo.Colored, blockworld) || block.getType() == main.WorldUtils.getInfo(MatInfo.LandMine, blockworld)) {
                        Reloadable = true;
                    }
                    if (Reloadable && isOk(block, Active) && block.hasMetadata(main.mInactive) == false) {
                       
                        setReloadingFlags(waveuid, block, start, Active, Inactive, wavespeed, duration, Distance, Explode, ExplosionForce, Larbin);
                       
                        if (!zone) {
                            nr.add(blocklocation);
                            for (Location loc : getNearBlocks(block.getLocation(), getType.PLANE)) {
                                List<Location> tempz = BlockRenewer(start, loc.getBlock(), Active, Inactive, duration, Distance, area, wavespeed, waveuid, Explode, ExplosionForce, Larbin, zone);
                                if (!tempz.isEmpty()) {
                                    nr.addAll(tempz);
                                }
                            }
                           
                            if (!main.WorldUtils.getInfo(BooleanInfo.Planear, blockworld)) {
                               
                                for (Location loc : getNearBlocks(block.getLocation(), getType.NONPLANE)) {
                                    BlockRenewer(start, loc.getBlock(), Active, Inactive, duration, Distance, area, wavespeed, waveuid, Explode, ExplosionForce, Larbin, zone);
                                    List<Location> tempz = BlockRenewer(start, loc.getBlock(), Active, Inactive, duration, Distance, area, wavespeed, waveuid, Explode, ExplosionForce, Larbin, zone);
                                    if (!tempz.isEmpty()) {
                                        nr.addAll(tempz);
                                    }
                                }
                            }
                        }
                    }
                }
            } else {
                String reloadUUID = UUID.randomUUID().toString();
                block.setMetadata(main.mRUUID, new FixedMetadataValue(plugin, reloadUUID));
                BlockReload(duration, Distance, block, Active, Inactive, reloadUUID);
            }
            return nr;
        }
     
  13. Offline

    Betagear

    Still not any idea ? The error isn't coming from my code, as you may have seen it.
    (kind of a bump)
     
  14. Offline

    mythbusterma

    @Betagear

    The error is definitely in your code, and your code is impossible to reason about. We can't help you other than to say debug and rearrange your code so that it is possible to read.

    Code should never look like this for this exact reason. You can't figure out where your error is.

    Also of note, the nondescript method names and static fields from other classes compound the difficulty.
     
    Mrs. bwfctower likes this.
  15. Offline

    Betagear

    @mythbusterma Just look at that :
    Code:
    if (!zone) {
                            nr.add(blocklocation);
                            for (Location loc : getNearBlocks(block.getLocation(), getType.PLANE)) {
                                List<Location> tempz = BlockRenewer(start, loc.getBlock(), Active, Inactive, duration, Distance, area, wavespeed, waveuid, Explode, ExplosionForce, Larbin, zone);
                                if (!tempz.isEmpty()) {
                                    nr.addAll(tempz);
                                }
                            }
                          
                            if (!main.WorldUtils.getInfo(BooleanInfo.Planear, blockworld)) {
                              
                                for (Location loc : getNearBlocks(block.getLocation(), getType.NONPLANE)) {
                                    BlockRenewer(start, loc.getBlock(), Active, Inactive, duration, Distance, area, wavespeed, waveuid, Explode, ExplosionForce, Larbin, zone);
                                    List<Location> tempz = BlockRenewer(start, loc.getBlock(), Active, Inactive, duration, Distance, area, wavespeed, waveuid, Explode, ExplosionForce, Larbin, zone);
                                    if (!tempz.isEmpty()) {
                                        nr.addAll(tempz);
                                    }
                                }
                            }
                        }
    This code is always passed, but after a certain amount of times, it stops working.
    What should I do ? should I create a class for it and stop getting a lot of result remashed into the previous class to be then sent to the first one ?
     
  16. Offline

    BreezerFly

    You need to split it up into methods and then add comments so that we know what your trying to do. Also need to see the methods you made that you're calling in this method.
     
  17. Offline

    Betagear

    @BreezerFly Error busted : Used a class with a dedicated List instead of chaining a lot of times the functions, and it worked.
    This error was due to the chaining of the same function a lot of times, always adding more locations to a list, wich was then sent to the parent.
     
  18. Offline

    BreezerFly

    Yeah, as we said further up, you should make it an iterative function instead of a recursive one
     
Thread Status:
Not open for further replies.

Share This Page