An Error I don't understand.

ID:140704
 
Oct 4 2009, 10:35 pm (Edited on Oct 4 2009, 10:40 pm)
 
proc/Found_Most(var/list/L)
  if(!L||!L.len) return
  var/list/found=list()
  var/mostfound=1
  var/foundmost=L[1]
  for(var/v in L)
    found[v]++ //This line has the error
    if(found[v]>mostfound)
      mostfound=v
      foundmost=found[v]
  return foundmost


I marked the line that has this runtime error:

runtime error: list index out of bounds
proc name: Found Most (/proc/Found_Most)
source file: Years.dm,111

Help is appreciated thanks.

I didn't make this proc and don't fully understand what it does, so I can't fix it.

And if that is not the problem, here is an example of how I used it:

mob/proc/Spawn_Check()
    var/list/Zs=new
    for(var/mob/P)
        Zs+=P.z
    var/Spawn_Z=Found_Most(Zs)


It checks what z plane of the map all the players are on, adds their z variable to a list. Then Found_Most() is supposed to return the z plane that has the most players.
Oct 4 2009, 10:40 pm
Dragonn wrote:
  var/list/found=list()
  found[v]++


found is an empty list, so trying to access something in nothing, logically won't work.
Oct 4 2009, 10:43 pm
In response to Schnitzelnagler
Maybe you can help me get a better functioning proc to do what I'm trying to do?

I thought it was weird how that list didn't actually seem to be used for anything.

Here is what I am trying to do, lets say I have this list: list(1,2,3,4,4,4,4,4,4,4,4,4,5,6,7,8,9)

Obviously there are a lot more 4s than anything else.

But how do I make a proc that will return a "thing" found most in a list?
Oct 4 2009, 10:53 pm
In response to Dragonn
Like that code Garthor gave you, except he made that little mistake. found[v]++ won't add v to the list like a normal assignation would. So you need to either add it to the list first if it isn't already there...
if(!found.Find(v)) //ha ha found.Find().
   found += v

...or change that line to a normal assignation instead, which does the above automatically: 
found[v] = found[v] + 1 //(if v isn't in found, found[v] just gives null)


BTW, it's not complex code, you can figure it out if you read through it carefully a couple of times. Basically, what it does is counting how many times each element appears in the given list (this is stored as associated values in the found list) and then constantly sets the most common element found yet's value and occurrence to the foundmost and mostfound vars, accordingly (what an evil naming scheme).
Oct 4 2009, 10:58 pm
In response to Kaioken
I'm still a bit confused since I don't know the purpose of the found list in the first place, so I don't know where that line of code you showed me goes. But here is what I have so far, is this correct?

proc/Found_Most(var/list/L)
  if(!L||!L.len) return
  var/list/found=list()
  var/mostfound=1
  var/foundmost=L[1]
  for(var/v in L)
    found[v]=found[v]+1
    if(found[v]>mostfound)
      mostfound=v
      foundmost=found[v]
  return foundmost
Oct 4 2009, 11:01 pm
In response to Dragonn
The line you changed is indented improperly, but other than that it should be fine.
Well, it shouldn't, actually, because Garthor made another small mistake. Because of that evil, confusing naming scheme, he got confused himself; the values assigned in these lines should actually be the other way around.
mostfound=v
foundmost=found[v]


Fixed:
mostfound=found[v]
foundmost=v
Oct 4 2009, 11:02 pm
In response to Dragonn
 
proc
    Found_Most(var/list/L)
        if(!L || !L.len)
            return
        else
            var
                found[] = new
                mostfound
                . = L[1]
            for(var/v in L)
                found["[v]"]++
                if(found["[v]"] > mostfound)
                    . = v
                    mostfound = found["[v]"]

mob
    verb
        Test()
            src << "[Found_Most(list(4,4,4,4,4,5,6,7,8,9,9,9, 10,10,10,10,10,10))]"


Would work, though, you have some problems in you design.
What should happen if there are some values that appear equally often?

Edit:
Gosh, while I was still compiling and rewriting the code in DM, Kaioken already posted again :p

I am too slow. *looks at Kaioken rushing by with a 'Meep Meeep - roadrunner sound*
Oct 4 2009, 11:16 pm
In response to Schnitzelnagler
That seems to work, though I don't claim to understand it all.

Thanks very much both of you.
Oct 4 2009, 11:18 pm
In response to Schnitzelnagler
Schnitzelnagler wrote:
I am too slow. *looks at Kaioken rushing by with a 'Meep Meeep - roadrunner sound*

Meep m-

Anyway, hey, your post was only a minute later, and was worth it at any case, seeing as it actually made me think of about a different problem with the code that I missed before, that your code covered - and that was the initial problem he had with the code instead of what I actually said (doing L[elem]++ does add it to the list if it isn't already there): the proc doesn't support lists with numerical values, as they can't be used as keys in associative lists (and that's what the proc would try to do).

Onto that...
Although saving the elements as text strings like you have solves that, I would do it differently, to make the proc as robust as possible - make sure you only embed numbers in text, and you should attach a unique identifier to that resulting text string too while you're at it, to differentiate it from a potential actual text string that's named the same, in case the proc was given a mixed list that might actually contain values such as 2 and "2". 
            for(var/v in L)
                var/key
                if(isnum(v))
                   key = "!numkey!_[v]"
                else key = v

                found[key]++
                if(found[key] > mostfound)
                    . = v
                    mostfound = found[key]


If you're wondering "why not just embed every value in text?", well, for some values other than numbers it could be problematic. If you're given a list of objects, for instance, embedding them will result in their name (possibly prefixed with "The "), which means multiple objects with the same name could end up using the same key in the list, which you don't want. Also, some built-in object types give identical results between different objects when they're embedded in text (some more obscure ones actually give an empty string back). So you should only embed the values you have to, and it causes no adverse effects in the case of numbers.
Oct 4 2009, 11:30 pm
In response to Kaioken
Kaioken wrote:
Meep m-

*Howls like a coyote and prepares from some painful cartoon death*


Kaioken wrote:
L[elem]++ does add it to the list if it isn't already there): the proc doesn't support lists with numerical values

Tried to express this with my very first post, but obviously failed miserably.


Kaioken wrote:
Although saving the elements as text strings like you have solves that, I would do it differently, to make the proc as robust as possible

I would handle this different as well, but since there was no proper design decision provided, I decided to try and be fast, solving just the posters problem, before that evil Kaioken-runner would reply.
Everything else would require a proper design document by the OP, displaying choices for the logic problems that appear before going into detail with making it fit every possible input.
Oct 5 2009, 4:16 pm
In response to Schnitzelnagler
I wouldn't understand a more fully featured version anyway. Though it'd be nice to see it.
Oct 5 2009, 7:46 pm
In response to Dragonn
Which would require some design decisions on your side though.

For example, how should the procedure notify you of failure?
Trivial! Return 0, you say? Not really. What if the list you're sorting contains zero as a valid numerical value?
So... make it... -1! Same issue. Your list could contain negative numbers just as well.
Likely enough the only proper solution would be to return two values. One showing success/failure and another for the result.

How do you treat multiple heaps?
If you check a list like (1,1,1,2,3,3,3), what should the function return? Failure, 1, 3, or (1,3)?

How would you like similar looking input to be handled?
For example, if I provide this list (1,2,"2","2","3","3") and ask it to be evaluated, what should the function return?
Does the numeric value 2 equal the string "2"?

That are just some of the problems the function has to deal with and the results are subject to your need.
You should really try and understand the code that was provided though, else you did not learn and only solved one problem at hand. Even more so as the code snippet is very simple.

I tried to add some comment, like I should have done to begin with.

proc
    Found_Most(var/list/L)
    // We defined a procedure and the argument it takes
        if(!L || !L.len)
            return
        // If the provided argument is not a list,
        // or if it is a list, but an empty one,
        // notify the caller of failure.
        // Note the clever trick that is being used here.
        // Because of BYOND applying short circuit evaluation
        // http://en.wikipedia.org/wiki/Short-circuit_evaluation
        // there can not be a runtime error if L is not a list,
        // even though we are checking it's length.
        else
            var
                found[] = new
                // Initialise an empty list
                mostfound
            . = L[1]
            // Set the default return value to first list element
            for(var/v in L)
            // Loop through the list we are searching
                found["[v]"]++
                // For each number we find, we create an associative
                // list entry in the (previously empty) list that we 
                // defined earlier. As there can not be multiple  equal
                // key values (an associative list can not contain the
                // key value 2 twice), whenever we encounter the same
                // value again, we simply add up to it's associated value
                // instead of creating a new one.   
                if(found["[v]"] > mostfound)
                // Now, if the associated value is greater than the value
                // of any previous key, we found one that we should memorize
                    . = v
                    // Memorize the value, by assigning it to the default return
                    mostfound = found["[v]"]
                    // And note that we have to look for this amount instead now


Oct 5 2009, 8:57 pm
In response to Schnitzelnagler
Just note, they're not all actually really design decisions.

Schnitzelnagler wrote:
Which would require some design decisions on your side though.

For example, how should the procedure notify you of failure?
Trivial!

Yes, pretty much; =P you can just leave it as it is by returning null, though you'd need to alter the proc to ignore null values in the list, which probably made sense to do in the first place anyway. Or, you could leave null counting in if you want and just designate a specific return value that means the proc failed, which can be any uncommon value whatsoever:
#define FAILURE (-361.17)
proc/Proc(data)
   if(!data) return FAILURE

mob/verb/test()
   if(Proc() == FAILURE)
      src << "the proc failed!"

And if you were paranoid and wanted the value to be completely bulletproof (more like tankproof) in case the proc was actually fed the value -361.17 (or whatever), you could create a totally-unique value just for this purpose, for example a long unique string or a type path of a node defined just for this purpose.
This is all really implementation details, not design. No matter which implementation you choose, the black box remains virtually identical to its user.

How do you treat multiple heaps?
If you check a list like (1,1,1,2,3,3,3), what should the function return? Failure, 1, 3, or (1,3)?

The only real design decision one has to make here, but not a hard one. Especially if we're going all the way to make the proc a useful general, global function, then as far as general utility goes, what's returned should be the list, which gives the caller the option to do whatever he wants with the results given. If the caller only cares about grabbing one of the identically-occurring values, then so be it, it can just grab one arbitrarily from the list.

How would you like similar looking input to be handled?

IMO, not an actual design decision since you should be able to safely answer that with the natural/default answer: as similar (which isn't equal). Not really any decision to make, since there wouldn't be any reason to do it otherwise, and the proc is intended for counting same values.
Oct 5 2009, 11:55 pm
In response to Kaioken
Kaioken wrote:
Just note, they're not all actually really design decisions.

Well, since English is not my native tongue, I often lack the concrete term and I am forced to use a more general one that is rather close.
Something I regret, but difficult to change.


Kaioken wrote:
Yes, pretty much; =P you can just leave it as it is by returning null, though you'd need to alter the proc to ignore null values in the list, which probably made sense to do in the first place anyway.

*confesses*
I was confused and thought that the default return value would be zero instead of null when writing that posting.


Kaioken wrote:
bulletproof

Good keyword. I forgot to check the list for being a list. ;)
if(!L || !istype(L,/list) || !L.len)


As for the rest, I would generally agree with your statements.
Oct 6 2009, 3:12 am (Edited on Oct 6 2009, 8:03 pm)
In response to Kaioken
So, would you be content with this implementation?

/*
Format:
    Found_Most(List)
Args:
    List: List to check for the most frequent element(s).
    Found_Most() requires a list as argument that does not contain null
Returns:
    null on failure, or a list with the values found most often
Example:
mob
    verb
        Test()
            var/list/K = Found_Most(list(5, 5, 5, 6, 6, 7, 7, 7))
            if(!K || !istype(K,/list) || !K.len)
                src << "There has been an error."
            for(var/v in K)
                src << "[v]"

This example displays:
5
7
*/

proc
    Found_Most(var/list/L)
        if(!L || !istype(L,/list) || !L.len)
            return
        else
            for(var/v in L)
                if(isnull(v))
                    return
            var
                found[] = new
                nfound[] = new
                temp[] = new
                mostfound = 1
            for(var/v in L)
                if(isnum(v))
                    nfound["[v]"]++
                else
                    found[v]++
            for(var/v in found)
                if(found[v] == mostfound)
                    temp += v
                else
                    if(found[v] > mostfound)
                        mostfound = found[v]
                        temp.Cut()
                        temp += v
            for(var/v in nfound)
                if(nfound[v] == mostfound)
                    temp += text2num(v)
                else
                    if(nfound[v] > mostfound)
                        mostfound = nfound[v]
                        temp.Cut()
                        temp += text2num(v)
            return temp


Edit:
I choose to create a new posting instead of editing my previous one because of the automatic notification, or it's lack when editing and thus trouble on catching the attention of the person being replied to.

Edit2: I was dumb, so I decided to rewrite the procedure.
The code features slight copy and pasting for the sake of speed (I would normally prefer a modular attempt, but considered it a waste here.