Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TryGetValue is now being used when getting WeaponInfo instances #14111

Merged
merged 1 commit into from
Oct 14, 2017

Conversation

SirJson
Copy link
Contributor

@SirJson SirJson commented Oct 3, 2017

A YamlException is thrown if TryGetValue fails. Closes #13692.

Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Just a couple of small requests:

DetonationWeaponInfo = rules.Weapons[DetonationWeapon.ToLowerInvariant()];
WeaponInfo thumpDamageWeapon;
WeaponInfo detonationWeapon;
var thumpDmgWeapToLower = ThumpDamageWeapon.ToLowerInvariant();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: avoid removing arbitrary letters from variable names. Saving 5 characters on a throwaway local variable isn't worth the hit to readability. thumpDamageWeaponToLower and detonationWeaponToLower are fine. If you really want to use shorter names then thumpName and detonationName would also be fine considering the context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using (ThumpDamageWeapon ?? string.Empty).ToLowerInvariant() here and in the other cases where the weapons aren't optional to avoid a NullReferenceException if the mod yaml overrides it to be empty (which translates to a null value).

@pchote
Copy link
Member

pchote commented Oct 8, 2017

Can you please also squash the commits once you're done?

….Weapons

Removed invalid spacing at the end of the line 36 in ThrowsShrapnel

Prevented NullReferenceException in cases where weapons aren't optional
@SirJson SirJson force-pushed the trygetvalue-weaponinfo-instances branch from 532d0fa to 459ae19 Compare October 9, 2017 21:34
@SirJson
Copy link
Contributor Author

SirJson commented Oct 9, 2017

Fixed up everything and squashed my commits. I'm using now the more verbose variant for the local variables for more consistency with the rest of the code.

@reaperrr reaperrr merged commit f2b5040 into OpenRA:bleed Oct 14, 2017
@reaperrr
Copy link
Contributor

changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants