FiendsOfTheElements
Make "Starting AoE Caster Item" guarantee that an AoE attack magic item exists (if possible)
I think this will work, but it's hard for me to test. The scenario is "we generated a random set of spells for item magic, none of which happened to include a spell in the 'AoE attack' set". This code should, in that case, pick a random AoE attack spell and add it to the list to be applied to items.
It's arguable that if it does so it should also remove a random spell from the list so that the total number of items with magic does not change.
does this actually fix an error that fails to generate a seed? or is it just cleanup?
the additional layer of abstraction by creating a separate method, instead of just containing all the relevant code in GetRandomAoe seems like a step backwards on first read.
the check "if (Spells.Where(spell => spell.IsAoEAttack()).Count() < 1) {" broke my brain a bit. so if there are no AoE attack spells (when would this happen?), you're forcing one? i'll have to look again with fresh eyes because i don't understand
does this actually fix an error that fails to generate a seed? or is it just cleanup?
The seed does not fail to generate, but it can generate a seed with no AoE attack spells on items, and then cannot place one into the starting inventory. If you check the "Starting AoE Caster Item" flag the expectation is that you will always get one; if you wanted to sometimes not get one, you would have made it tristate.
the additional layer of abstraction by creating a separate method, instead of just containing all the relevant code in GetRandomAoe seems like a step backwards on first read.
Rather than have the exact same (and necessarily identical) logic in two places it's better to encode it in one canonical location and call it twice. SpellHelper seemed like the logical place.
the check "if (Spells.Where(spell => spell.IsAoEAttack()).Count() < 1) {" broke my brain a bit. so if there are no AoE attack spells (when would this happen?), you're forcing one? i'll have to look again with fresh eyes because i don't understand
Yes, the logic is "the (potentially random) set of spells selected for item magic did not include at least one AoE attack spell".
What highqualityshawn kind of wanted to hint at is that if (Spells.Where(spell => spell.IsAoEAttack()).Count() < 1) isn't the simplest method of doing what you want.
It would probably be easier to understand to do if (Spells.Where(spell => spell.IsAoEAttack()).Count() == 0).
But Count() has to enumerate the whole sequence, which isn't a big deal in this case since there's a limited amount of spells.
So the way you normally do this would be if (!Spells.Any(spell => spell.IsAoEAttack())). Any stops the iteration at the first element that matches the function.
Not a big deal, but doesn't hurt to know...
Also please don't change line endings or reformat whole files if you can help it. It makes reviewing changes very hard.
Also please don't change line endings or reformat whole files if you can help it. It makes reviewing changes very hard.
Nothing should have changed line endings (I have it set to checkout and commit as-is), but I had to reformat that file in order to even read it. The mixed tabs and spaces made it impossible to tell what was going on. That's why I did the whitespace-only changes in a separate commit, so when looking at it commit-by-commit it's still easy to see the difference.
What highqualityshawn kind of wanted to hint at is that if (Spells.Where(spell => spell.IsAoEAttack()).Count() < 1) isn't the simplest method of doing what you want.
It would probably be easier to understand to do if (Spells.Where(spell => spell.IsAoEAttack()).Count() == 0).
But Count() has to enumerate the whole sequence, which isn't a big deal in this case since there's a limited amount of spells.
So the way you normally do this would be if (!Spells.Any(spell => spell.IsAoEAttack())). Any stops the iteration at the first element that matches the function.
Not a big deal, but doesn't hurt to know...
I'm not that familiar with C#, and did not realize that their list class provides Any. It makes sense that it would, now that I think about it.
I can change this as you suggest.