[1.8] Trying to Remove Vanilla Recipes

[Entevily]

[pre]I'm using Forge for Minecraft 1.8.

 

So I'm trying to access the Crafting Manager and remove recipes for certain vanilla items.  Below is the code I've got so far.

 

[/pre]

public static void removeRecipes() {
CraftingManager cm = CraftingManager.getInstance();
List recipes = Lists.newArrayList();
recipes = cm.getRecipeList();
for(int i = 0; i < recipes.size(); i ++){
	if(recipes.get(i) instanceof ShapedRecipes){
		if(((ShapedRecipes)recipes.get(i)).getRecipeOutput().getItem().equals(Items.stone_axe)){
			System.out.println("SoTrue");
			cm.getRecipeList().remove(i);
		}
	}
}
}

[pre]

Problem is, that this prints the line "SoTrue" but it does not remove the item at that index.  I removed the 'cm.getRecipeList().remove(i)' and replaced it with 'cm.getRecipeList().clear()' and all recipes go out the window.

 

So what might I be doing wrong, when I'm just trying to remove, like in this example, the Stone Axe Recipe.  I'm calling this method in the Init phase of my common proxy.

 

Any assistance with this will be greatly appreciated.  Thanks.

[/pre]

[diesieben07]

Don't use manual indexing with a for loop. Use an Iterator.

[Draco18s]

Or if you do, count down. Because you removed an element, the next one is at the current index, which you increment before looking at a new element.

[RANKSHANK]

I'd definitely recommend learning to use iteration, as it's cleaner looking, easier to debug, and harder to mess up

/**removes the specified Item's corresponding recipes*/
public static void removeCraftingRecipes(Item target){
	List<IRecipe> l = new ArrayList();
	for(IRecipe r : (List<IRecipe>)CraftingManager.getInstance().getRecipeList())
		if(r.getRecipeOutput() != null &&  r.getRecipeOutput().getItem() == target)
					l.add(r);
	for(IRecipe r : l)
		CraftingManager.getInstance().getRecipeList().remove(r);
}

Make sure you do not iterate through a list and modify it at the same time, as a general practice. Here I'm copying the recipes I want to remove when iterating through the list, and then remove them after they are all found. This prevents comodification errors because the lists iteration access ends with the for loop, allowing for it to be accessed again to call remove()

for(Object<T> o: Iterable<T> i)
// The iterable's <T> must be safely castable to the object's <T> (Of course objects can't have a <T> but for the example's sake)
// eg:  
for(String s : new String[] {"a", "b"});
//or
for(Object o : new String[] {"a", "b"});
//but NOT
for(String s: new Object[] {"a", "b"});

Iteration is a wondrous thing, basically the first section is your allocated object which has its value fed from the array/list in the second section. The loop is then run through and the variable is replaced by the next item in the array/list until you use a break, or the list/array is exhausted. Notice how I cast CraftingManager.getInstance.getRecipeList() to a List<IRecipe> so I can iterate through as an IRecipe array

[LexManos]

You're talking about an enhanced for loop (sometimes referred to as a ForEach loop)

What dies meant was to use an iterator:

itr = lst.iterator
while (itr.hasNext)
  item = itr.next
  if should remove item: 
    itr.remove

Enhanced forloops are just syntax sugar for that that hides the iterator, but that doesn't allow you to use the remove function which is the important part.

This method allows you to not have to pass over the list twice and keeps track of your index in the list for you.

[RANKSHANK]

Definitely ignore what I said-

 

I've always avoided instantiating my own iterators in favor of the shorthand variant... Thanks for the heads up Lex!

[LexManos]

If you need to access the iterator itself, use it manually, if you don't and just want to loop over everything use foreach, they both have their plusses and minuses just like everything in programming. You have to decide which one you need to use.

[Earthcomputer]

Better to use Predicates here:

 

public void removeRecipe(Predicate<IRecipe> shouldRemove) {

  Iterator iterator = CraftingManager.getInstance().getRecipeList().iterator();

  while (iterator.hasNext())

      if (shouldRemove.apply((IRecipe) iterator.next()))

        iterator.remove();

}

 

Hope that helps

[Entevily]

Thanks everyone for your awesome responses.  All of you have great points.  I ended up using the solution provided by RankShank.

 

The only other thing I did was reverse the list before going through the remove phase so that I don't remove an index and then change the order of the rest and possibly accidently remove something else, like Draco18s had mentioned.

 

Here is my final code:

public static void removeRecipe(Item itemToRemove) {
	List<IRecipe> remove = new ArrayList();
	for(IRecipe recipe : (List<IRecipe>)CraftingManager.getInstance().getRecipeList())
		if(recipe.getRecipeOutput() != null &&  recipe.getRecipeOutput().getItem() == itemToRemove)
			remove.add(recipe);

	remove = Lists.reverse(remove);

	for(IRecipe r : remove)
		CraftingManager.getInstance().getRecipeList().remove(r);
}

[diesieben07]

...

Great, you did what everyone told you not to do.

[Entevily]

diesieben07, I do appreciate your response.  I understand that an Iterator was recommended.  The only issue is I do not have experience with an iterator.  I still have it in my comments in my code to look into using an iterator at a later point.  I had not been able to get an iterator to remove the recipe so I decided not to use it until I understand it.  I was, however, able to remove the recipes using the for-each loop.

 

But thanks for your response.

[diesieben07]

Lex even posted the code for you. You just have to translate it to Java, which is straightforward.

[yooksi]

Thank you for providing very useful information on this subject! I've been struggling a long time trying to properly remove recipes. After reading what you guys wrote here (especially Draco18s) I've finally managed to understand the problem I've been facing. I failed to account for the element movement in the list after we remove something thus I kept using the wrong index. This is what I've written so far, do you guys think this can be written any more elegant?

private static int removeRecipe(Item toRemove)
{
      int recipesRemoved = 0;
      List<IRecipe> recipeList = CraftingManager.getInstance().getRecipeList();

      // Iterate through the recipe list and find the recipes we're looking for.
      // Search using iterators instead of manual indexing to increase reliability.

      java.util.Iterator<IRecipe> recipeEntry = recipeList.iterator();
      while (recipeEntry.hasNext())
      {
      ItemStack outputItem = recipeEntry.next().getRecipeOutput();
      if (outputItem != null && outputItem.getItem() == toRemove)
      {
	      recipeEntry.remove();
	      recipesRemoved++;
      }
      }      return recipesRemoved;
}

Edit: improved the code a bit.