An inventory system with 3000 items ... really? Well, I've found several issues in the code. Please understand the following as constructive criticism:
1. Giving variables in a broader scope (and "global" is as broad as possible) unexpressive names like "n" and "d" isn't be of use for readability and memorization but name clashes. The names "selectionLogic" and "activator" are better in this sense. Naming the iteration variable "i" inside the very small scope of the for loop is (IMHO) okay.
2. Invoking "applySelectionLogic" inside the "draw" function mixes unrelated functionality (single responsibility principle).
3. When at the very left and still pressing the LEFT key, the counter in "activator" falls below zero. Similarly, when at the very right and still pressing the right key, the counter in "activator" exceeds the amount of places. Due to the missing limitations, one has to press the opposite key as often (or long when counting on key repetition) until the counter gets back in the valid range.
4. You initialize "activator" as a two element array, but accessed ever is only the first one.
5. BTW: Why is activator an array at all?
6a. This part "&&i<n" of the 1st conditional inside the "applySelectionLogic" is meaningless, because the outer loop already ensures this.
6b. This part "&&i>-1" of the 2nd conditional inside the "applySelectionLogic" is meaningless, because the outer loop already ensures this.
7. Probably, the issues 6a and 6b hints at an attempt to protect the access to "selectionLogic" (although not strictly necessary, but from a logical pint of view). In that case the both parts have to be exchanged and also made stricter: "i>0" and "i<n-1", respectively.
8. If you have a 1 out of N selection, why do you need to store it in an array of length N? EDIT: I mean, one does not want to iterate an array, hitting almost ever a "this item isn't selected" until in the end finding the wanted "this item is selected". Please don't be deceived due to the seemingly fitting of the array inside the "draw" function (s.a. issue 2).
9. The system is (at least on my computer) not very responsive. It pauses at will, but performs the meanwhile happened key repetitions shortly after. That makes it unusable.
Comments on the style: Although perfectly valid, comparisons with "false" and "true" are somewhat redundant. It is like querying "is it true that a condition is true?" instead of "is the condition true?". (And, BTW, why is "= " preferred to "new array()" in the means of best practices?)
The solution is much too complex for what it yields in. For a 1 out of N selection from an array, a simple index is necessary for storing the selected element. But perhaps it is just in preliminary stage? However ...
... I could use just one variable, but in this case, I would not know what to do if I had 900.000 items to select. ...
... in a case where it is to be expected that a few of many items are to be selected at the same time, one still would not do it that way. This has 2 reasons: (a) It were a waste of memory (899.990 items are not selected but require memory) and time (iterating the entire array), and (b) there is no way to store the order in which items were selected (a feature that often plays a role). So, the better solution in such a case would be to look at the whole as a "sparse selection", e.g. storing the indices of selected items in a variable length array.
Edited by haegarr, 08 October 2013 - 03:46 AM.