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

Search implementation #67

Merged
merged 6 commits into from
Dec 25, 2024
Merged

Search implementation #67

merged 6 commits into from
Dec 25, 2024

Conversation

jans510
Copy link
Contributor

@jans510 jans510 commented Dec 10, 2024

No description provided.

public List<PublicProfileDto> getDancersList(AuthenticatedUser authenticatedUser, Gender gender, int range) {

Dancer dancer = loadByUserId(authenticatedUser.getUserId());
Double longitudeRange = (double)range/112;
Copy link
Contributor

Choose a reason for hiding this comment

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

Das könnte ein Bug sein aber ich bin mir nicht sicher, die Division hier range/122 müsste eine Integer-Division sein, die dann als Output in ein Double gewandelt wird. Aber auch wenn das nicht der Fall ist würde ich das von der Konsistenz her eher so schreiben, wie in der Zeile darunter:

Suggested change
Double longitudeRange = (double)range/112;
Double longitudeRange = range/112.0;

public List<PublicProfileDto> getDancersList(AuthenticatedUser authenticatedUser, Gender gender, int range) {

Dancer dancer = loadByUserId(authenticatedUser.getUserId());
Double longitudeRange = (double)range/112;
Copy link
Contributor

Choose a reason for hiding this comment

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

Es hat ja vermutlich einen Grund also nur damit ich es auch verstehe: Warum wird hier die Double-Klasse genutzt und darunter double als primitiver Datentyp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hatte keinen Grund, ich habs gefixt

@@ -36,4 +39,23 @@ public HashMap<UUID, DancerDto> getDancerMap(DancerIdsDto dancerIdsDto) {

return dancers;
}

public List<PublicProfileDto> getDancersList(AuthenticatedUser authenticatedUser, Gender gender, int range) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ist nur eine Kleinigkeit, im Englischen würde man hier mMn Singular nehmen, getDancerList, z.B. eine Liste von Kandidaten wäre im Englischen candidate list, nicht candidates list.

public List<PublicProfileDto> getDancersList(AuthenticatedUser authenticatedUser, Gender gender, int range) {

Dancer dancer = loadByUserId(authenticatedUser.getUserId());
Double longitudeRange = (double)range/112;
Copy link
Contributor

Choose a reason for hiding this comment

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

Kannst du einen Kommentar schreiben wieso wir diese Divisoren genommen haben, damit wir das später noch wissen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Ich würde noch einen Test hinzufügen, dafür was passiert, wenn man range/gender nicht angibt. Sind die Werte dann null und kann das Backend damit umgehen? Wenn wir die beiden Such-Parameter als mandatory definieren, geben wir dann einfach eine leere Liste aus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ich hab für Range mal einen default value angesetzt (20 km). Gender würde ich als required vorsehen und einen bad request Treturnen, falls der nicht gesetzt ist

@jans510 jans510 merged commit da9654b into master Dec 25, 2024
1 check passed
@jans510 jans510 deleted the search-implementation branch December 25, 2024 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants