Skip to content

Commit

Permalink
Allow namechanges that don't change the userid anytime
Browse files Browse the repository at this point in the history
  • Loading branch information
@HoeenCoder
HoeenCoder committed Sep 21, 2018
1 parent 6024c4a commit 2a7ffcb
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion users.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -721,7 +721,9 @@ class User {
* @param {Connection} connection The connection asking for the rename
*/
async rename(name, token, newlyRegistered, connection) {
let userid = toId(name);
for (const roomid of this.games) {
if (userid === this.userid) break;

This comment has been minimized.

Sign in to view

Copy link
@Zarel

Zarel Sep 21, 2018

Member

This should probably be outside the loop.

This comment has been minimized.

Sign in to view

Copy link
@HoeenCoder

HoeenCoder Sep 22, 2018

Author Member

Its actually already handled below https://github.com/Zarel/Pokemon-Showdown/blob/master/users.js#L772

So I just caused the loop to exit if its not a proper namechange and let that handle it.

This comment has been minimized.

Sign in to view

Copy link
@Zarel

Zarel Sep 22, 2018

Member

Yes, you're checking it once per game, when you should only be checking once overall.

It's not like userid doesn't equal this.userid for the first game, but maybe on the third game userid will equal this.userid.

This comment has been minimized.

Sign in to view

Copy link
@Zarel

Zarel Sep 22, 2018 •
edited
Loading

Member

In other words, this should be refactored to

if (userid !== this.userid) {
	for (const roomid of this.games) {

It's a matter of having the code reflect the logic. For readability, something checked inside a loop should be something that has the potential of changing between loop iterations.

This comment has been minimized.

Sign in to view

Copy link
@HoeenCoder

HoeenCoder Sep 22, 2018

Author Member

In hindsight that makes more sense then this checking everytime when it evaluates to false (true breaks the loop obv)
const game = Rooms(roomid).game;
if (!game || game.ended) continue; // should never happen
if (game.allowRenames || !this.named) continue;
Expand All @@ -747,7 +749,6 @@ class User {
return false;
}

let userid = toId(name);
if (userid.length > 18) {
this.send(`|nametaken||Your name must be 18 characters or shorter.`);
return false;
Expand Down

0 comments on commit 2a7ffcb

Please sign in to comment.