Skip to content
This repository was archived by the owner on Jul 24, 2023. It is now read-only.

Adds the following feature.#93

Open
Rmoneygit wants to merge 1 commit into
masterfrom
user-following
Open

Adds the following feature.#93
Rmoneygit wants to merge 1 commit into
masterfrom
user-following

Conversation

@Rmoneygit

Copy link
Copy Markdown
Contributor

No description provided.

@Rmoneygit
Rmoneygit requested review from angplee and tgomezzzz August 1, 2020 01:33
User user = new User(keyString, name, profilePicKey);
followerUsers.add(user);
} catch(EntityNotFoundException e) {
e.printStackTrace();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If a user's key isn't found (maybe the user's account was deleted or something), then this try-catch will completely interrupt the servlet and return no followers to the user. I think this behavior can be improved with a continue statement, so the problematic user is skipped, but the loop still gets all the other value users.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it also might be better to run a query on Datastore users as opposed to iterating through a loop of keys and getting user data (I think individual calls to separate APIs are costly).

@tgomezzzz
tgomezzzz self-requested a review August 3, 2020 17:57
@Rmoneygit
Rmoneygit requested review from adamtcohen and vidhyal August 7, 2020 16:03

User user = new User(keyString, name, profilePicKey);
usersFollowed.add(user);
} catch(EntityNotFoundException e) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if possible, please auto-reformat before submitting. There are a lot of small formatting inconsistencies.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Feel free to ignore.

@adamtcohen adamtcohen left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Did a very quick review for major issues since it is the last day.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants