-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: master
Are you sure you want to change the base?
Conversation
...n/java/com/eternalcode/parcellockers/deliverycode/repository/DeliveryCodeRepositoryImpl.java
Outdated
Show resolved
Hide resolved
...n/java/com/eternalcode/parcellockers/deliverycode/repository/DeliveryCodeRepositoryImpl.java
Outdated
Show resolved
Hide resolved
if (this.cache.get(parcelUUID) != null) { | ||
return Optional.of(this.cache.get(parcelUUID)); | ||
} | ||
return Optional.ofNullable(this.findByUUID(parcelUUID).join()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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));
There was a problem hiding this comment.
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
...n/java/com/eternalcode/parcellockers/deliverycode/repository/DeliveryCodeRepositoryImpl.java
Outdated
Show resolved
Hide resolved
…itory/DeliveryCodeRepositoryImpl.java Co-authored-by: DMK <81445555+imDMK@users.noreply.github.com>
…itory/DeliveryCodeRepositoryImpl.java Co-authored-by: DMK <81445555+imDMK@users.noreply.github.com>
…itory/DeliveryCodeRepositoryImpl.java Co-authored-by: DMK <81445555+imDMK@users.noreply.github.com>
…itory/DeliveryCodeRepositoryImpl.java Co-authored-by: DMK <81445555+imDMK@users.noreply.github.com>
There was a problem hiding this 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<>(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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ę
@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 |
...n/java/com/eternalcode/parcellockers/deliverycode/repository/DeliveryCodeRepositoryImpl.java
Outdated
Show resolved
Hide resolved
Co-authored-by: DMK <81445555+imDMK@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm waiting for resolve:
As I said above, I can't simply make this PR a monster. |
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. |
Remove cache from repo impl |
Resolves #67