Start moving stuff to brig #1
Conversation
Peashooter101
left a comment
Peashooter101
left a comment
There was a problem hiding this comment.
Minor changes requested, should not hold up merge.
Remember to squash when merging and make the squashed commit meaningful.
| for (Player player : targets) { | ||
| FlyLogic.setFlyStatus(player, shouldEnable); | ||
| player.sendMessage(getParsedComponent(shouldEnable, player, sender, LocaleMessage.FLY_SET_BY_OTHER.getMessage())); | ||
| modified++; |
There was a problem hiding this comment.
There is no code path that would prevent modified from being incremented, meaning modified == targets.size().
You should either just use targets.size to set modified or introduce another code path where if it fails to set a player for some reason (I don't know if this condition even exists), it will not count them as "modified".
| if (shouldEnable) { | ||
| enabledString = LocaleMessage.ENABLED.getMessage(); | ||
| } else { | ||
| enabledString = LocaleMessage.DISABLED.getMessage(); | ||
| } |
There was a problem hiding this comment.
| if (shouldEnable) { | |
| enabledString = LocaleMessage.ENABLED.getMessage(); | |
| } else { | |
| enabledString = LocaleMessage.DISABLED.getMessage(); | |
| } | |
| enabledString = shouldEnable ? LocaleMessage.ENABLED.getMessage() : LocaleMessage.DISABLED.getMessage(); |
Perfect time to use a ternary, but this is a coding style.
| if (flyEnabled) { | ||
| enabledString = LocaleMessage.ENABLED.getMessage(); | ||
| } else { | ||
| Util.sendUserMessage(sender, ConfigValues.flySetOther, ConfigValues.disabled, player); | ||
| Util.sendUserMessage(player, ConfigValues.flySetByOther, ConfigValues.disabled, sender); | ||
| return true; | ||
| enabledString = LocaleMessage.DISABLED.getMessage(); | ||
| } |
There was a problem hiding this comment.
| if (flyEnabled) { | |
| enabledString = LocaleMessage.ENABLED.getMessage(); | |
| } else { | |
| Util.sendUserMessage(sender, ConfigValues.flySetOther, ConfigValues.disabled, player); | |
| Util.sendUserMessage(player, ConfigValues.flySetByOther, ConfigValues.disabled, sender); | |
| return true; | |
| enabledString = LocaleMessage.DISABLED.getMessage(); | |
| } | |
| enabledString = flyEnabled ? LocaleMessage.ENABLED.getMessage() : LocaleMessage.DISABLED.getMessage(); |
Perfect time to use a ternary, but this is a coding style.
| if (flyEnabled) { | ||
| enabledString = LocaleMessage.ENABLED.getMessage(); | ||
| } else { | ||
| enabledString = LocaleMessage.DISABLED.getMessage(); | ||
| } |
There was a problem hiding this comment.
| if (flyEnabled) { | |
| enabledString = LocaleMessage.ENABLED.getMessage(); | |
| } else { | |
| enabledString = LocaleMessage.DISABLED.getMessage(); | |
| } | |
| enabledString = flyEnabled ? LocaleMessage.ENABLED.getMessage() : LocaleMessage.DISABLED.getMessage(); |
Perfect time to use a ternary, but this is a coding style.
Draft PR