South-Paw / warframe-item-list

👾 A list of all Warframe items that contribute to player mastery rank
GNU General Public License v3.0
13 stars 5 forks source link

Naming is Inconsistent #24

Closed will-janz closed 6 years ago

will-janz commented 6 years ago

Some categories (for example, in constants.js) are singular, and others are pluralized. Files are also named inconsistently (zaw.js vs archwings.js).

Since this is a collection of collections, I'd vote pluralized file and variable names.

South-Paw commented 6 years ago

Hi 👋 thanks for raising this one!

With regards to the category constant names, I don't think they should all be pluralized as some don't make sense as plurals.

The constants are applied to single weapons rather to groups of weapons so Bows, Launchers or Daggers doesn't make sense to me if I was to get a weapon object:

{
  name: 'Example Rifle',
  acquisition: 'Some location',
  category: 'Rifles',
  masteryRank: 8,
},

However Dual Pistols makes sense as there are two of them (their pairs of course 😄 ) just like Claws do as well.

Most importantly though - all these category names are matching with the wiki weapon groups (see the weapon's type) and this was my original goal - so I think these are fine.

However, with the file names, I totally agree that they are inconsistent and trip my OCD now that you've pointed them out 👍

You've also made me realize additionally that the itemType constants are not used as the keys of the items.objects export so I'll fix this one too...

will-janz commented 6 years ago

Sorry, I wasn't properly clear. Category properties aren't what I meant, those are great the way they are. I meant the variable names. For example:

module.exports = {
  acquisition,
  acquisitionQuests,
  acquisitionFactions,
  acquisitionEnemies,
  primaryCategory,
  secondaryCategory,
  meleeCategory,
  itemType,
};

Seems inconsistent, since they're all sets.

module.exports = {
  acquisitions,
  acquisitionQuests,
  acquisitionFactions,
  acquisitionEnemies,
  primaryCategories,
  secondaryCategories,
  meleeCategories,
  itemTypes,
};

Seems more proper IMO. I'm working on the PR for this. Thoughts?

South-Paw commented 6 years ago

Hi, thanks for PR! 👍

I've merged into a dev branch and I'll fix the other thing I noted:

You've also made me realize additionally that the itemType constants are not used as the keys of the items.objects export so I'll fix this one too...