Assets/Script/Inventory

[EthanNichols]

Thoughts

Sorta difficult to review this much code. Mainly because there many more classes being referenced that would just unfold out into a huge system to learn. So while there are still things that I could suggest improving I decided to save myself from opening Pandora's box.

Formatting / Grammer

Don't really have a good understanding with what your comment formatting is. Seems like: //comment No space after //, Start with a lower case letter, No periods /// CommentSpace after ///, Start with upper case letter per line, Some assisting punctuation, Comment params and return values Just make sure comments follow whatever your formatting scheme is. Might help creating a formatting wiki on this project's wiki.

• [x] In Inventory.cs:232 There's a weird comment style. My personal preference for multi-line comments for a param:

  /// <param name="filter">
  /// Way to filter the returned list.
  /// -1 = no filter, 0-6 grabs equippables by the slot they belong in, 7 = all equippables, 8 = all battle items, 9 = all materials
  /// </param>

• [x] ItemTypeDefinitions.cs:5 Unhelpful class comment. Explain the purpose of the class or remove the comment

• [x] ItemTypeDefinitions.cs:12 //How much one of this item sells for -> //How much a single item sells for

• [x] ItemTypeDefinitions.cs:49 & ItemTypeDefinitions.cs:66 I suggest utilizing in comments. Which makes it more obvious if you're talking about another class that exists rather than the idea of a Pawn

• [x] ItemTypeDefinitions.cs:62 //Is this item can be used outside of battle -> //Whether this item be used outside of battle

Suggestion(s):

• [x] ItemTypeDefinitions.cs:56 Instead of using an int create an enum for the equipmentslot

• [ ] ItemTypeDefinitions.cs:62 Seems like this should be an enum as well. Can't tell though

• [x] Inventory.cs:14 Might want to inherit IComparable or IComparer

• [ ] Inventory.cs:40 Might want to look into == operator overloading

• [x] Inventory.cs:154 & Inventory.cs:156 Magic file location. Make this a constant at the top of the class or come up with a better way of finding/creating/reading folders. Since it's not the best practice to use literal directories, but use relative directories.

• [ ] Inventory.cs:165-207 I think this is mostly Debug code, but there's a lot of magic variables here. I would strongly suggest moving constant values like the strings to a more accessible place to be edited. Then use the variable names instead. Just prevents future instances where you're searching for these values inside of functions.

• [x] Inventory.cs:219 I would strongly suggest making this a regular writer rather than a binary writer. First of all it's binary, so people will be able to reverse engineer it pretty quickly. The main benefit to have it readable-friendly is for debugging purposes. A suggestion would be to use macros so that for a built game a binary writer is used, and for playing in the editor a normal writer is used.

• [x] Inventory.cs:235 I would suggest making the filter an enum

• [ ] Inventory.cs:243 & Inventory.cs:384 Look into copy constructor(s) then the code will be new StoredItem(i) This will also clean up the inheritance problem which will require Equippable to also have it's own copy constructor. But it will read much cleaner and by less ambiguous.

• [ ] Inventory.cs:322 Function seems redundant. Just add to the static list from where ever the AddItem function is being called.

• [x] Inventory.cs:395 Not sure how you're doing other if statements. I know I prefer to just have all if statements with { } braces on seperate lines. Even if there is only 1 line inside them.

Questionable:

I would keep 1 class per file. Reduces confusion when you're looking at Inventory.cs, but looking at the StoredItem class.

I also like to keep Enums to their own file. But that's a personal preference.

• [x] Inventory.cs:391 I think it's slightly more efficient to just sort in the reverse order rather than "forward" sorting then reversing the sorted list. Also just use the SortingType enum as the parameter instead of an int. Code below isn't tested, but it should have the same affect.

  public static void SortInventory(SortingType sorting)
  {
  if (sorting == SortingType.AmountDecreasing)
  {
      itemList.Sort((x, y) => x.CompareTo(y));
  }
  else
  {
      itemList.Sort((x, y) => y.CompareTo(x));
  }
  }

[Jacob-Brosler]

Fixed most of the issues, here's the reasoning for what wasn't fixed: ItemTypeDefinitions.cs:62 - Due to the nature of this value denoting different things depending on this being a weapon or armor there's no real way to have a cohesive enum make sense.

Inventory.cs:40 - Unsure of how that would help

Inventory.cs:165-207 - Pure debug, no reason to migrate from strings as it will be removed when I'm done testing inventory.

Inventory.cs:243 and Inventory.cs:384 - I could be wrong but even with a copy constructor I'd still need to cast to equippable for it to copy correctly, leading to no real change in complexity. I did add copy constructors to make it so if I change either of the classes later it's less searching to change the copier.

Inventory.cs:322 - I'd rather anything editing the static list be in the inventory class if it can be helped, also consolidates addition for later in case more functionality needs to be put there.

I'll leave this open for now, if you disagree with any of this please let me know.

[EthanNichols]

ItemTypeDefinitions.cs:62 I would suggest trying to make sure the variable only represents one thing. Rather than an assortment of different things. Up to you though.

Inventory.cs:40 There was a "CompareTo" function which appears to have been removed.

Inventory.cs:322 If you only want to add/remove/edit the list in the inventory class then there is an inherit problem with having the list be static. If you don't want someone to open a door, don't put a doorknob on it.

Inventory.cs:384 Yes you'd still have to cast it. But it's more scalable for the future using a copy constructor. Unless there is only some specific data that you'd like copied instead of the entire class.