Celebration / Issue with loot

[Shaggy]

So first off, gooooo me! Ha. I got my map systems working, and I have a [very primitve] FoV system in as well! Check it out! Also, let me know what you think of the HUD? Couldn't think of anything else you'd need, but I may have missed something.

http://img684.imageshack.us/img684/704/60110216.png


Now, on to my issue. For some reason, even if there's plenty of room in the players inventory, it says the inventory is full when you try to pick things up off the map. Any idea why? Here's the code [vb.net] for my pickup routine.

Code: [Select]

Public Sub GetItem(ByVal left As Integer, ByVal top As Integer)

        Dim intloop, intinnerloop As Integer
        Dim picked As Boolean

        picked = False

        For intloop = 0 To 2
            If Not LootMap(Player.Left, Player.Top, intloop).Name = "Nothing" Then

                For intinnerlooploop = 0 To 25
                    If Player.Inventory(intinnerloop).Name = "Nothing" Then
                        WriteAt(1, 0, ConsoleColor.White, "Picked up a ")
                        WriteAt(13, 0, ConsoleColor.Yellow, LootMap(left, top, intloop).Name)
                        WriteAt(40, 0, ConsoleColor.Black, "")

                        Player.Inventory(intinnerloop) = NOITEM
                        Player.Inventory(intinnerloop) = LootMap(left, top, intloop)
                        LootMap(left, top, intloop) = NOITEM
                        picked = True
                    Else
                        WriteAt(1, 0, ConsoleColor.Red, "No free inventory space!")
                    End If
                Next

            Else



            End If
            If picked = True Then Exit For
        Next


    End Sub
EDIT ::

I've also done debugging to make sure that the system knows the item is there, which it does, and that it's in the right position on the map.  The inventory[0 to 25] array shows a ton of empty items, and for some reason it still says there's no space.

I triple checked the spelling, as well as trying to check with a different value. But I get the error nonetheless.

[guest509]

 For intinnerlooploop = 0 To 25
                    If Player.Inventory(intinnerloop).Name = "Nothing" Then


  Shouldn't those 2 variables match?

[Shaggy]

Ahhh! Thank you 

[Z]

It also seems that when e.g. you have items at positions [ 0] to [6] and [25], it displays the "No free inventory space!" 7 times, then "Picked up X" one time, then "Picked up Nothing" 17 times (for values from 8 to 24), then "No free inventory space!" one time. I.e., the loop does not stop after finding the first empty position, and displays "No free inventory space!" for each used position.

[Krice]

A funny example how long variable name can be confusing and cause a bug! intinnerloop is a stupid variable name anyways. Learn to use names that tell something! In this case I guess there are 25 (26?) inventory slots. So, better variable name could be inventory_slot, if you insist using long variable names for loops.

[RogueMaster]

A funny example how long variable name can be confusing and cause a bug! intinnerloop is a stupid variable name anyways. Learn to use names that tell something! In this case I guess there are 25 (26?) inventory slots. So, better variable name could be inventory_slot, if you insist using long variable names for loops.

I can even tell more: among professional programmers (I mean, programmers that work on an enterprise, etc...) there is a standard name for things when programming, and how to write code. For example:

· Constant = All letters Upper case: TRUE, FALSE, YES, NO, etc... There is a constant that does not follows this always: __VERSION__
· Variables = descriptive name. If made of two or more words, the first word is lowercase, the other words uppercase the first letter: numOfItems, playerHealth, inventorySlots, mapLevel, etc...
· Procedures = Like variables, but uppercase the first word: SendMessage(), PrintMap(), MoveMonsters(), ...

But it's really important to use descriptive names, so the maintenance of the code is done easier. Don't use variables very similar, because it can confuse when writting code. But not with similar letter, also with similar meanings, can lead to confussion: numOfItem is almost like numberOfItem, but also numOfObjects, numOfItems, numOfThings, can lead to cofussion.

And avoid variables like 'a', 'm', 'var1', 'var2', etc... unless just used into a loop like "For i = 1 to 10", etc...
But it seems that there is a certain variable that doesn't follows this always: 'aux', that some people may use as a wildcard for some situations that needs an universal variable, and don't want to declare a new one because they just need to store some data somewhere but for just a single operation that doesn't needs a new variable.

Also comments are extremely usefull (and almost a have-to), because the next time you review your code you can remember what that it does exactly, because after tousand lines of code, it's hard to remember how such piece of code works exactly.

This is also very important so you allow other people to read your code without much problem.

[Shaggy]

Thanks everyone! I got it all worked out, including that pesky loop issue 

Here's the new code, if anyone's interested

Code: [Select]

Public Sub GetItem(ByVal left As Integer, ByVal top As Integer)

        Dim intloop, intinnerloop, ItemNum As Integer

        'Steps through to find the first item in the stack
        For intloop = 2 To 0 Step -1
            If Not LootMap(Player.Left, Player.Top, intloop).Name = "Nothing" Then
                'sets Itemnum to represent the items spot in the stack
                ItemNum = intloop
                Exit For
            End If
        Next

        'Steps through the players inventory
        For intinnerloop = 0 To 25
            'Finds the first empty inventory
            If Player.Inventory(intinnerloop).Description = "Empty space." Then
                'Display a 'Picked up a [ITEM]' message
                WriteAt(1, 0, ConsoleColor.White, "Picked up a ")
                WriteAt(13, 0, ConsoleColor.Yellow, LootMap(left, top, ItemNum).Name)
                WriteAt(40, 0, ConsoleColor.Black, "")

                'Put the item in the players first empty inventory spot,
                Player.Inventory(intinnerloop) = LootMap(left, top, ItemNum)
                'Take the item off the lootmap
                LootMap(left, top, ItemNum) = NOITEM
                Exit For
            Else
                'if there's no space, display a message
                WriteAt(1, 0, ConsoleColor.Red, "No free inventory space!")
                Console.Title = Player.Inventory(intinnerloop).Name & " " & intinnerloop

            End If
        Next

    End Sub

[Z]

The second loop still does not seem to do what was intended...

[Shaggy]

Why's that? It's working fine haha

[Krice]

· Variables = descriptive name. If made of two or more words, the first word is lowercase, the other words uppercase the first letter: numOfItems, playerHealth, inventorySlots, mapLevel, etc...
· Procedures = Like variables, but uppercase the first word: SendMessage(), PrintMap(), MoveMonsters(), ...

iThinkIt'SHardToReadSomethingLikeThis, this_style_is_better_and_more_readable.

[Z]

OK, the "Exit For" should do the trick. However, I think the line "No free inventory space!" is still displayed as many times as you have items in your inventory (before an empty space is found). Probably it does not matter because it is then covered by the correct message and you do not see it, but still... it would be better for your learning of programming to have your program work in a logical way. Something like (I don't know how exactly a while loop is done in Basic)

Code: [Select]

a = 0
while a < 26 and Player.Inventory(a).Description = "Empty space." do
  a=a+1
wend

if a=26 then 
                WriteAt(1, 0, ConsoleColor.Red, "No free inventory space!")
                Console.Title = Player.Inventory(intinnerloop).Name & " " & intinnerloop
else
                'Display a 'Picked up a [ITEM]' message
                WriteAt(1, 0, ConsoleColor.White, "Picked up a ")
                WriteAt(13, 0, ConsoleColor.Yellow, LootMap(left, top, ItemNum).Name)
                WriteAt(40, 0, ConsoleColor.Black, "")

                'Put the item in the players first empty inventory spot,
                Player.Inventory(a) = LootMap(left, top, ItemNum)
                'Take the item off the lootmap
                LootMap(left, top, ItemNum) = NOITEM
endif

Also, I think the loop variable should have an one-letter name, and it is usually better to define a constant instead of using number 26.

[Rabiat]

Shaggy:

I'm assuming you're using VB.NET. Some advice:

• It's good practice to enable Option Strict, enable Option Explicit, and disable Option Infer. It's a chore to explicitly declare and initialize all variables, but it'll prevent a slew of hard to detect runtime bugs.
• Like Übermann, I'd encourage you to use descriptive variable names and abide to coding conventions. In .NET, stick to lowerCamelCasing for variables and UpperCamelCasing for classes and methods; it's what everyone does.
• .NET has terrific support for variable size collections, such as List(of T) or Dictionary(of TKey, TValue). If you use these, there's no need to check for absence of items from a loot list, or inventory slots with nothing in them.
• If you have reason to ignore the previous point, just don't pull stunts like If Inventory(i).Description = "Empty space." or LootMap(...).Name = "Nothing". If you change the description/name of the 'empty' item, these tests will fail. It's safer to assign Nothing to an empty slot, and test for Inventory(i) Is Nothing or LootMap(...) Is Nothing. It's best not to have empty slots at all, though.
  
• It's usually a good idea to separate 'model' code from 'view' code, i.e. separate the code that actually moves an item from the floor to the inventory, from the code that displays a message. In any case, don't display a message saying the player has picked up an item before the item is actually moved. If somehow the item can't be moved after all, the message is confusing.
  

Other than that I think .NET is a good choice for developing a roguelike. It'll help you create a game, in stead of winding up with the classic Yet Another Incredibly Abstracted Roguelike Library Only I Will Ever Use. ;)

[Shaggy]

Shaggy:

I'm assuming you're using VB.NET. Some advice:

• It's good practice to enable Option Strict, enable Option Explicit, and disable Option Infer. It's a chore to explicitly declare and initialize all variables, but it'll prevent a slew of hard to detect runtime bugs.

So what exactly does that do? I've seen it in a lot of projects, but never used it for a simple lack of knowledge.

• Like Übermann, I'd encourage you to use descriptive variable names and abide to coding conventions. In .NET, stick to lowerCamelCasing for variables and UpperCamelCasing for classes and methods; it's what everyone does.

I usually do, it's just I use things like intLoop for variables that are used only for loops.

• .NET has terrific support for variable size collections, such as List(of T) or Dictionary(of TKey, TValue). If you use these, there's no need to check for absence of items from a loot list, or inventory slots with nothing in them.

Another thing I've seen around, but lack the knowledge of how to use.

• If you have reason to ignore the previous point, just don't pull stunts like If Inventory(i).Description = "Empty space." or LootMap(...).Name = "Nothing". If you change the description/name of the 'empty' item, these tests will fail. It's safer to assign Nothing to an empty slot, and test for Inventory(i) Is Nothing or LootMap(...) Is Nothing. It's best not to have empty slots at all, though.

I tried doing things like Is Nothing, or = NOITEM [My empty item], but it pulls up an error, and the way I'm doing now works fine for the time being at least

• It's usually a good idea to separate 'model' code from 'view' code, i.e. separate the code that actually moves an item from the floor to the inventory, from the code that displays a message. In any case, don't display a message saying the player has picked up an item before the item is actually moved. If somehow the item can't be moved after all, the message is confusing.

So you think I should make a seperate sub or function just to write that they picked up an item? I think putting it in with the sub that handles picking up items makes perfect sense

Other than that I think .NET is a good choice for developing a roguelike. It'll help you create a game, in stead of winding up with the classic Yet Another Incredibly Abstracted Roguelike Library Only I Will Ever Use. ;)

Thanks! I've used vb6 a lot, but that's pretty outdated. And I haven't programmed period in a while either, so I figured starting a basic roguelike and improving on it as I go would be a good way to oil the gears, if you will.

[Ex]

· Variables = descriptive name. If made of two or more words, the first word is lowercase, the other words uppercase the first letter: numOfItems, playerHealth, inventorySlots, mapLevel, etc...
· Procedures = Like variables, but uppercase the first word: SendMessage(), PrintMap(), MoveMonsters(), ...

iThinkIt'SHardToReadSomethingLikeThis, this_style_is_better_and_more_readable.

IPreferCamelCase.

[Z]

• .NET has terrific support for variable size collections, such as List(of T) or Dictionary(of TKey, TValue). If you use these, there's no need to check for absence of items from a loot list, or inventory slots with nothing in them.

I think that, if the inventory has a fixed size of 26 items, using an array of size 26 with 'nothing' in some of them is OK (if you assign a letter to each item, which is 'a' plus its index, then the letters won't change even if items in between are dropped, which is good IMO). However, I don't understand why a floor slot would have a fixed size of 3, so either allow only 1 item per floor slot, or use a List or something.

But checking for nothingness by comparing item name to "Nothing" is indeed very ugly. In fact, any checking for item/monster type by comparing strings is ugly. It's best to consider strings as data which can be changed easily (e.g., for the purpose of translation) and thus their exact values should not appear everywhere in your code, only in 1 place. Either use a "type" field which is an integer value, or use an Object Oriented approach, where every item type is a different class.

• It's usually a good idea to separate 'model' code from 'view' code, i.e. separate the code that actually moves an item from the floor to the inventory, from the code that displays a message.

So you think I should make a seperate sub or function just to write that they picked up an item? I think putting it in with the sub that handles picking up items makes perfect sense

No, create a ShowMessage procedure, which displays a message (any message, not only "you pick up an item") to the user. This has many advantages compared to the current solution: it makes changing the layout of the game much easier, and you can also easily remember all the messages so that you can see them with a "show previous 50 messages" key, or log them to an external file. However, there is one disadvantage: it is much harder to do syntax highlighting, that is, displaying parts of the message in different colors (you need to do some design to have both syntax highlighting and the advantages above). Since you already have syntax highlighting, I suppose this could be a problem.