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

GH-85 Add delivery codes #85

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

GH-85 Add delivery codes #85

wants to merge 20 commits into from

Conversation

Jakubk15
Copy link
Member

@Jakubk15 Jakubk15 commented Oct 15, 2023

Resolves #67

@Jakubk15 Jakubk15 changed the title Add delivery codes GH-85 Add delivery codes Oct 15, 2023
@Jakubk15 Jakubk15 marked this pull request as ready for review October 22, 2023 11:28
igoyek
igoyek previously approved these changes Oct 22, 2023
vLuckyyy
vLuckyyy previously approved these changes Oct 22, 2023
Comment on lines 81 to 84
if (this.cache.get(parcelUUID) != null) {
return Optional.of(this.cache.get(parcelUUID));
}
return Optional.ofNullable(this.findByUUID(parcelUUID).join());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (this.cache.get(parcelUUID) != null) {
return Optional.of(this.cache.get(parcelUUID));
}
return Optional.ofNullable(this.findByUUID(parcelUUID).join());
Optional<DeliveryCode> deliveryCodeOptional = Optional.ofNullable(this.cache.get(parcelUUID));
if (deliveryCodeOptional.isPresent()) {
return deliveryCodeOptional;
}
return this.findByUUID(parcelUUID).join();

Copy link
Member

Choose a reason for hiding this comment

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

ten optional słabo wygląda

Copy link
Member Author

Choose a reason for hiding this comment

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

To mam revertować te optionale?

Copy link
Contributor

@imDMK imDMK Oct 22, 2023

Choose a reason for hiding this comment

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

You can also do this:
Optional.ofNullable(this.cache.get(parcelUUID)).or(() -> this.findByUUID(parcelUUID).join));

Copy link
Member

Choose a reason for hiding this comment

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

**sorry, że piszę po polsku zapomniałem, że nie wolno.

PL: Ogólnie to pakowanie w optionala w tym samym scope i go sprawdzanie mija się z celem np.:
EN: Packing an optional in the same scope and checking it is pointless

Optional optional = Optional.ofNullable(getNullable());

if (optional.isEmpty()) {
    // ....
}

PL: to co zrobił DMK jest już dobre
EN: what DMK has done is already good

return Optional.ofNullable(this.cache.get(parcelUUID)).or(() -> this.findByUUID(parcelUUID).join));

3,
PL: Robienie tutaj join mija się z celem i jest też kompletnie bez kontekstu (bez use case)
EN: join() here is pointless and is also completely without context (without use case)

EN: Cache should only be in the worst possible case. please remove it

…itory/DeliveryCodeRepositoryImpl.java

Co-authored-by: DMK <[email protected]>
Jakubk15 and others added 3 commits October 22, 2023 14:00
…itory/DeliveryCodeRepositoryImpl.java

Co-authored-by: DMK <[email protected]>
…itory/DeliveryCodeRepositoryImpl.java

Co-authored-by: DMK <[email protected]>
…itory/DeliveryCodeRepositoryImpl.java

Co-authored-by: DMK <[email protected]>
@Jakubk15 Jakubk15 requested a review from imDMK October 22, 2023 12:01
Copy link
Member

@Rollczi Rollczi left a comment

Choose a reason for hiding this comment

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

giga jest dużo code style zmian, a samego kodu biznesowego nie widzę... nawet nie ma tutaj use case...

there are a lot of code styles, I can't see the change in the business code itself... there's not even a use case here...


public class DeliveryCodeRepositoryImpl extends AbstractDatabaseService implements DeliveryCodeRepository {

private final Map<UUID, DeliveryCode> cache = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

ja bym cache nie dodawał dopóki nie będzie realnego usecase gdzie będzie to potrzebne

Copy link
Member Author

Choose a reason for hiding this comment

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

Będzie realny use case bardzo niedługo, na razie zostawiam, w razie co to usunę

@Jakubk15
Copy link
Member Author

@Rollczi Nie ma use case, bo delivery code mają być generowane przy dojściu paczki do Lockera, a nie ma zrobionych jeszcze timed tasków i eventów pod to, więc póki co tworzę sobie obiekty i bazę danych, żeby w przyszłych PR nie było potworków. Wszystkie zmiany related do kodów odbioru są w paczce deliverycode. Pozdrawiam cieplutko

@Jakubk15 Jakubk15 requested a review from Rollczi October 22, 2023 20:06
@Jakubk15 Jakubk15 requested a review from imDMK October 22, 2023 20:32
Copy link
Member

@Rollczi Rollczi left a comment

Choose a reason for hiding this comment

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

@Jakubk15
Copy link
Member Author

@Rollczi Nie ma use case, bo delivery code mają być generowane przy dojściu paczki do Lockera, a nie ma zrobionych jeszcze timed tasków i eventów pod to, więc póki co tworzę sobie obiekty i bazę danych, żeby w przyszłych PR nie było potworków. Wszystkie zmiany related do kodów odbioru są w paczce deliverycode. Pozdrawiam cieplutko

As I said above, I can't simply make this PR a monster.

@Jakubk15
Copy link
Member Author

Jakubk15 commented Oct 23, 2023

The Optional you requested has been changed. I believe that using cache with #join will be used really carefully and in necessary situations, as I already did in many GUIs.

@Rollczi
Copy link
Member

Rollczi commented Oct 23, 2023

@Rollczi Nie ma use case, bo delivery code mają być generowane przy dojściu paczki do Lockera, a nie ma zrobionych jeszcze timed tasków i eventów pod to, więc póki co tworzę sobie obiekty i bazę danych, żeby w przyszłych PR nie było potworków. Wszystkie zmiany related do kodów odbioru są w paczce deliverycode. Pozdrawiam cieplutko

As I said above, I can't simply make this PR a monster.

Remove cache from repo impl

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.

Add delivery codes
5 participants