WIP - Lowercase cleanup #1706
WIP - Lowercase cleanup #1706
Conversation
Tests needs to be adapted accordingly
| } | ||
|
|
||
| purgeService.purgePlayers(sender, namedBanned, bannedPlayers.toArray(new OfflinePlayer[bannedPlayers.size()])); | ||
| purgeService.purgePlayers(sender, namedBanned, bannedPlayers.toArray(new OfflinePlayer[0])); |
There was a problem hiding this comment.
Please revert.
I know it's an IntelliJ warning but why should we tell Java to create an array generically by reflection when we literally know the size beforehand?
| @@ -66,7 +66,7 @@ public void resetCount(String address, String name) { | |||
| if (isEnabled) { | |||
| TimedCounter<String> counter = ipLoginFailureCounts.get(address); | |||
| if (counter != null) { | |||
| counter.remove(name); | |||
| counter.remove(name.toLowerCase()); | |||
| @@ -20,7 +20,7 @@ | |||
| * @param auth the player auth object to save | |||
| */ | |||
| public void updatePlayer(PlayerAuth auth) { | |||
| cache.put(auth.getNickname().toLowerCase(), auth); | |||
| cache.put(auth.getNickname(), auth); | |||
There was a problem hiding this comment.
Are you sure about this one? Aha, nickname is lowercase by definition. OK.
| if (savedCode != null && savedCode.equalsIgnoreCase(code)) { | ||
| captchaCodes.remove(nameLowerCase); | ||
| captchaCodes.remove(name); |
There was a problem hiding this comment.
I think this one needs to be lowercase.
| boolean result = source.removeAuth(name); | ||
| if (result) { | ||
| cachedAuths.invalidate(name); | ||
| cachedAuths.invalidate(name.toLowerCase()); | ||
| } | ||
| return result; |
| } | ||
| } else { | ||
| if (sender != null) { | ||
| commonService.send(sender, MessageKey.ERROR); | ||
| } | ||
| ConsoleLogger.warning("An error occurred while changing password for user " + lowerCaseName + "!"); | ||
| ConsoleLogger.warning("An error occurred while changing password for user " + playerName + "!"); |
There was a problem hiding this comment.
This is a nice cleanup in this class. Thanks
| final String ip = PlayerUtils.getPlayerIp(player); | ||
|
|
||
| if (service.getProperty(RestrictionSettings.UNRESTRICTED_NAMES).contains(name)) { | ||
| if (validationService.isUnrestricted(name)) { |
| @@ -90,7 +90,7 @@ public void run() { | |||
| //we went through all offlineplayers but there are still names remaining | |||
| for (String name : toPurge) { | |||
| if (!permissionsManager.hasPermissionOffline(name, PlayerStatePermission.BYPASS_PURGE)) { | |||
| namePortion.add(name); | |||
| namePortion.add(name.toLowerCase()); | |||
There was a problem hiding this comment.
Good. Code later on requires namePortion being lowercase-only.
| PlayerAuth auth = PlayerAuth.builder() | ||
| .name(playerNameLowerCase) | ||
| .name(playerName) | ||
| .realName(playerName) |
There was a problem hiding this comment.
.name(playerName)
.realName(playerName)This has bit of a funny feel now. It's absolutely 100% correct. I'm just wondering if it'd make sense to have a single name(String) function, since obviously from the realname we immediately know the all-lowercase name? This is maybe something we can look at after this pull request (for the sake of merging this soon). I expect that in some cases we do only want to set the name only (without a realname), but in most cases it would be nice if we just had to pass the name once and could tell the PlayerAuth builder "hey you, do what you need to do" 😄
Edit: So to be clear, my suggestion is to do nothing about it for now but we should keep it in the back of our heads.
There was a problem hiding this comment.
yeah, we can enhance it in a future cleanup
|
Now the problem is updating the unit test xD |
Tests needs to be adapted accordingly, any feedback?