Help with making code more effective and efficient

Hi All,

First time poster, apologises in advance for bad formatting, I have only been using P5.js for about 2 weeks now. I have had previous knowledge of how code works from my programming course at college. 5 years on from that and I’ve decided to pick it back up as a hobby again and learn a new language.

I have made the below basic inventory system that you can drag and drop items from different slots to others. and drag the entire inventory by clicking on the top bar. This is a system that in my teenage years I always wanted to create but was never able to. this became my first goal for my first project.

Inventory System Beta V1

I have 2 main class files Inventory.js and Item.js (at the bottom of Item.js you can make Item type classes that extend off Item.js. this was mainly for me to understand extension of classes.)

the size of the inventory is all modular and you can change this on line 18 with the last 2 arguments of where the inventory object is created.

There is a function in the main sketch.js called testAddItem() where i have been adding items in the code to test the inventory… all the items are taken from a sprite map.

the project is in a state where I have been unable to get any errors. However the cases for removing too much of an item or adding past the stacklimit of an item has been handled to not break the node, but doesn’t yield any output messages to the user as this is just currently a concept.

My main question is to people who take a look at this is: Is there anything glaring obviously that I should be doing differently when coding in general?

TLDR: Made inventory System, How can i make the code better?

Thanks

Peachman

1 Like

Hi! and welcome! :smiley: , just noticed that it seems you keep developing on the same linked resource, so when others go to take a look, things might not be in a working state / halfway between 2 features! (this might be discouraging for people to try and debug to get the code into a working state, and then comment on the structure of your code), also when you do get suggestions, and implement the changes, others that come by to read the thread might not follow the thread as easily!

for starters, it seems like you have a fairly good grasp of coding already (you mentioned you already had some experience from 5 years ago, and it shows!), and as such, there aren’t that many obvious mistakes that pop up (in contrast to those that are completely new to coding).

In general, asking for something specific is much more likely to yield answers (I just noticed that nobody answered over the last few days), and it really boils down to the vague “is there something wrong?” type of question!

some ‘naming’ nitpicking:

  show() {
    if (this.showing) {

here I would maybe choose a better word than ‘show’ for your class method that does the rendering, maybe draw or render are good alternatives. Especially when you then use the word ‘showing’ in the next line, as the state for whether the class is to be drawn or not.

if (this.showing) {

I like to mirror the name draw() library function in my class methods (reducing the vocabulary of my program… even though show() is a perfectly valid name!).

another name I don’t really like is overBar:

if (this.overBar) {

it’s inside the Inventory, but it means that “the mouse is over the drag-bar”, so I would name it mouseOverBar.

another one is locked:

function mouseDragged() {
  if (locked) {
    inventory.dragMove();
  }
}

here I think it means “the inventory is locked to the mouse”, so I would name it inventoryLockedToMouse, but I would also consider moving the variable into the Inventory class, since it only seems to relate to the inventory. Once that’s done, the need for “inventory” in the variable name becomes redundant, and the name instead becomes “lockedToMouse” :slight_smile: .

I hope this gives a window into how I think when I go about naming things, but it’s not easy…
It’s not without good reason when people say “the two hardest things in programming is ‘naming things’, ‘cache invalidation’, and ‘off by 1’ errors”. Naming comes with practice, and I’m by no means an expert so don’t think the names I chose are “the only right ones”, I’d instead focus on the process of naming. (and it’s so easy to be opinionated about naming)

comments

code ‘decisions’ as comments

 //did bug here once? cannot get it go again.

(assuming that this comment alludes to a ‘bug that happened’ at some point in time, but after a change in your code, it stopped happening)

ok, normally I wouldn’t even mention ‘git’ on this forum, as processing/p5js is mostly for ‘quick prototyping’ (and fun), but given that you’re a bit experienced already, and the vagueness of the question, I think this might be worth at least mentioning! but If you end up wanting to grow this application to something bigger, and maybe come back to it in the past, or share it with a friend, then ‘git’ becomes an inevitable subject/hurdle. Some type of comments just ‘disappear’ when you start writing them as ‘changes / decisions over time’, you made the decision at “some point in time”, and it belongs to “some piece of code”, and the place to write a comment linking a decision to the change is… VCS! (version control system) I called it ‘git’ earlier, but that’s just the most common form of VCS out there!

another type of ‘code decision’ as comment is the famous ‘commented out code’:

     //item = new Item(itemSheet, 16, x, y, itemID - 1);
      //items.push(item);

They have a tendency to ‘stick around’ more than they tend to ever be uncommented again, so once you “make the decision” to “not have this code execute anymore”, just delete it, and let the code exist in your VCS!

comments as code (aka. “repeating what the code does”)

comments such as:

 //draw rectangle top bar

should be “written as code”, if a function is becoming too long to ‘read easily’, and feel like they necessitate a comment to describe what it’s doing, it’s very likely that the function itself has become too large, and is doing too many things, here I would split this function up into the following:

drawTopBar() {
   ...
}

drawItemCells() {
  ...
}

drawInventory() {
  this.drawTopBar();
  this.drawItemCells();
}

It might seem like “extra code I have to write, and the comment is just 1 line, whereas a function definition and call requires at least 4 lines!..”, but wait! there are some benefits!

  1. code never gets “outdated”. Sometimes a comment is left behind when the code changed, and it leads to more confusion than if it hadn’t been there to begin with!
  2. it’s easier to “see the scope” of related code, the code that “draws the top bar” is separate from code that “draws the inventory”
  3. code is never wrong, or lies to you, it simply does
  4. code is interactive! (you can run it and see what happens, and step through it with a debugger)
  5. refactoring becomes easier, swap out the whole function, or move the function somewhere else! (this point kinda relates to the point about “scope”)

standing on the shoulders of giants (#storytime)

Now yes, I just wanted to use that phrase in the title ;), but we really do “stand on the shoulders of giants” when it comes to code (as with most things), languages come with standard functions, thousands of libraries simply exist, conventions are formed! (and subsequently not followed!), but it’s a wonderful place to be!

I mention this because I struggle with this myself (still), but at some point on my programmer journey I told myself in a sort of strict ‘finger wagging’ way: “spend a lot of time researching your first moves!, it’s not worth it to start writing code too early!”, I fondly remember going off and wanting to “write code!”, and sure I knew how to apply the knowledge I had just learnt from university, and went off creating stuff since “this is what I was trained to do!.. writing code!”

Not once did I stop to think “… what is out there?!.. what do other people do?..” (the worst part about this story is that I sometimes STILL do this!, despite having supposedly “learnt my lesson”).

to make this story relevant here, when you have code like this addItem (and also removeItem):

addItem(item, quantity) {
    //print("ADDITEM")
    this.item = item;
    var added = false;
    //if item is already in the inventory, add to its quantity.
    for (var i = 0; i < this.inventoryCell.length; i++) {
      if (this.inventoryCell[i][2]) { //if there is something in cell
        if (this.inventoryCell[i][3].ID === this.item.ID) {
          this.inventoryCell[i][3].incimentStack(quantity); //this does return a true of false depending on if the stack was met.
          added = true;
          break;
        }
      }
    }
    //if the item was not in the inventory, add it to the next empty slot.
    if (!added) {
      for (i = 0; i < this.inventoryCell.length; i++) {
        if (this.inventoryCell[i][2] === false) { //in the cell is empty
          this.inventoryCell[i][2] = true; //cell is now full
          this.inventoryCell[i][3] = this.item; //we set index 3 of the cell to the item.
          this.inventoryCell[i][3].incimentStack(quantity) //and we then incriment the item by that amount.
          added = true;
          break;
        }
      }
    }
  }

It might be worth noting that Javascript has some good standard Array functions you can take advantage of, such as find(), indexOf(), and findIndex(). The last one (findIndex()) might come in real handy if you had an array of items, and you wanted to iterate through them to find a specific one! and if you restructured your 2D array to be just a single array of data, you’ll find that you might be able to cut down on a large amount of code this way. (all that 2D array iteration, and 2D array access code)

In this case, you can separate the way you want something to look (a 2D grid of items), from the way you want to represent it in code (a single array of items). This is often hard to do on the fly, and perhaps easier in retrospect. But in general it can be worth thinking about these 2 things separately, and giving them an equal share of thought! (they’re both important, and a change in one can make the entire problem a lot simpler).

Now I’m not saying that doing this refactoring to a 1D array is “the right way” to go for this code (in fact, it might turn out to be complicated for both drawing & mouse interaction, going both from 2d -> 1d (mouse click coordinates), and 1d -> 2d (drawing cell from index to canvas coordinates)), but it’s the thought that counts! (and the incentive is “how can I utilize existing functions like findIndex() here?”)

2 Likes

HI Julian,

Thanks for taking the time to have a look at this small project of mine, it really means a lot. I will definitely take this advice on board. I have parked this inventory project for the moment to start on something new that i want to pull together with this and other projects to simply learn different aspects of coding., But will come back to this later this week (got some holiday this week to hopefully learn some more!)

I would like to mention about the comments. They’re just comments!!! But all seriously though, I didn’t realise how important they are. i think this might be because of the lack of confidence i have when coding, as well as general just house keeping of the code.

When i was coding the addItem and removeItem, i was thinking that there must have been a better way of doing it. I will look into trying to use the array functions you said to try and reconstruct this section (will be the 3rd time now but that’s how we learn!)

Thanks again for the feedback! this was exactly what I was looking for!

Thanks

Ryan

1 Like

Glad it was helpful! I was worried about the quality of my post, whether it made any sense, was easy to read… etc… and I know it was probably way too lengthy (I wrote it pretty late that day, and… I seem to always browse this forum past midnight… :crescent_moon: ).


I have parked this inventory project for the moment to start on something new that i want to pull together with this and other projects to simply learn different aspects of coding.

Ultimately coding is about scrapping projects! (and maybe coming back to them later!)


When i was coding the addItem and removeItem, i was thinking that there must have been a better way of doing it.

Yeah! the subconscious is usually a really good indicator for when something is due for a refactor, or when to start looking for a better solution. Sometimes you have to deal with a sub-par solution there and then, but it often comes back to you sometime later, maybe after waking up the next day (always keep a notebook ready!).

1 Like