Breaking a loop.

ID:144354
 
Jan 3 2007, 5:33 pm
Code:
if (Result2 == "Potion of LongLife")
                                if(M in view(1,src))
                                    for(var/obj/Items/Potions/S in M)
                                        if(S.IsEmpty == 1)
                                            for(var/obj/Items/gems/Diamond/F in M)
                                                for(var/obj/Items/Silks/SpiderSilk/T in M)
                                                    var/fail
                                                    fail = prob(M.PotionSkill*4)
                                                    if(fail == 0)
                                                        usr<< "Failed!"
                                                        M.weight -= F.weight
                                                        del F
                                                        M.PotionSkill += 1
                                                        M.weight -= T.weight
                                                        del T
                                                        break
                                                        return
                                                    S.IsPotion = 1
                                                    S.IsEmpty = 0
                                                    S.icon_state = "RedPotion"
                                                    M.PotionSkill += 2
                                                    S.name = "Potion of LongLife"
                                                    M.weight -= T.weight
                                                    M.weight -= F.weight
                                                    del F
                                                    del T
                                                    break
                                                    return


Problem description:

As you can see this code makes a potion :) , but the problem is that for some reason it takes away two or more diamonds where as it should only take one away, is there anyway to fix this by any chace? I've tried using "break" but no effect.
Jan 4 2007, 12:46 am
Oh dear... All those breaks are doing are sending you back outside the 'for(var/obj/Items/Silks/SpiderSilk/T in M)' - to the 'for(var/obj/Items/gems/Diamond/T in M)' loop, causing it to be consume another diamond.

What I tend to do with loops like this is put a Label at the bottom, and at the end of each loop of code, stick in a 'goto Label' line.
Jan 4 2007, 12:58 am
In response to Dead_Man_Running
Dead_Man_Running wrote:
What I tend to do with loops like this is put a Label at the bottom, and at the end of each loop of code, stick in a 'goto Label' line.

Eew. You can just use while() y'know.
Jan 4 2007, 1:05 am
In response to Android Data
That's a very good point, Android. *goes back to code to change it*
Jan 4 2007, 1:05 am
The break instructions breaks the innermost loop. In this case, it's breaking your loop through /obj/Items/Silks/SpiderSilk and not the diamonds. You can remedy this by putting a label inside your code and then breaking the label.

                                    for(var/obj/Items/Potions/S in M)
                                        if(S.IsEmpty == 1)
                                            diamond_loop:
                                                for(var/obj/Items/gems/Diamond/F in M)
                                                    for(var/obj/Items/Silks/SpiderSilk/T in M)
                                                        ...
                                                        break diamond_loop


You're also making two mistakes:
  • The return behind the break is useless and will just waste space sitting there. It's unnecessary since the break immediately halts execution of further code below.
  • I have suspicions - though I may be mistaken - that S.IsEmpty is a boolean. If this is the case, you don't need to do if(S.IsEmpty==1) nor if(S.IsEmpty==0) but should instead do if(S.IsEmpty) and if(!S.IsEmpty). They do the same things but are cleaner to use, widely accepted among the developers and they prevent future errors (what if S.IsEmpty is somehow set to 2? if(S.IsEmpty) would normally return while your version would think it's not empty).


  • I don't know about a possible third mistake: for(blah in M) rather than for(blah in M.contents). I suppose it's a matter of preference, but I believe M.contents is more accepted.
Jan 4 2007, 1:08 am
A few problems:

  • You're using == 1 for what appears to be a boolean variable (IsEmpty). Instead, use Boolean Shortcuts. Look up the ! operator. It literally means NOT. !! would be NOT NOT, but an even number of NOTs cancel each other out, so if(!!IsEmpty) would translate to if(IsEmpty), meaning if IsEmpty is true.
  • What's with the 'fail' variable? You can just cut out that variable altogether and do if(prob(M.PotionSkill*4))?
  • The break negates the return.
Jan 4 2007, 1:43 am
In response to Dead_Man_Running
Ah thank you, you have all been very helpful, that will solve alot of my coding problems now :)