#11 Cleaning redis service and adding ttl getter method

Birleştirildi
jonathan 4 yıl önce kris/refactor_redis_service içindeki 4 işleme master ile birleştirdi
kris 4 yıl önce yorum yaptı
Henüz bir içerik yok.
kris başlık WIP: (testing) Cleaning redis service iken Cleaning redis service adn adding ttl getter method olarak değiştirildi 4 yıl önce
kris 4 yıl önce yorum yaptı
Poster

@jonathan please review

@jonathan please review
jonathan 4 yıl önce kris tarafından atandı
kris başlık Cleaning redis service adn adding ttl getter method iken Cleaning redis service and adding ttl getter method olarak değiştirildi 4 yıl önce
jonathan 4 yıl önce yorum yaptı
Sahibi

@kris -- Things that make me wary:

  • use of var in a low-level library. The lower you go, the more explicit types should be. Why introduce another potential source of errors? Can we remove var and replace with class names?
  • use of streams/lambdas/collecting and in general anywhere we are now creating new objects when we could have avoided it. low level libraries should generally prefer for-loops and static classes. Can you review and avoid using stream methods and allocating new objects where possible?

Please give me a heads-up in advance before refactoring more library code. With the above changes I will merge this, but frankly I don’t see a big benefit given the risk. If we had more comprehensive test coverage I would feel better about the risk.

@kris -- Things that make me wary: * use of `var` in a low-level library. The lower you go, the more explicit types should be. Why introduce another potential source of errors? Can we remove `var` and replace with class names? * use of streams/lambdas/collecting and in general anywhere we are now creating new objects when we could have avoided it. low level libraries should generally prefer for-loops and static classes. Can you review and avoid using stream methods and allocating new objects where possible? Please give me a heads-up in advance before refactoring more library code. With the above changes I will merge this, but frankly I don't see a big benefit given the risk. If we had more comprehensive test coverage I would feel better about the risk.
kris 4 yıl önce yorum yaptı
Poster
  • Removing var (only String was replaced in 5ish places, so it was easy) - agree, sorry
  • stream/lambda/collect is used just in 1 place - method prefix(Collection<String>). In master (current version) following is used transform(keys, o -> prefix(o.toString())) - so lambda is there also. I did some perfomance test on this, and the new solution worked 2x faster at least on collections with 10.000 Strings. Please confirm you really want apache’s transform method back. Or maybe to just change all this and use plain for loop with another collection created as output?
- Removing var (only String was replaced in 5ish places, so it was easy) - agree, sorry - stream/lambda/collect is used just in 1 place - method `prefix(Collection<String>)`. In master (current version) following is used `transform(keys, o -> prefix(o.toString()))` - so lambda is there also. I did some perfomance test on this, and the new solution worked 2x faster at least on collections with 10.000 Strings. Please confirm you really want apache's transform method back. Or maybe to just change all this and use plain for loop with another collection created as output?
jonathan 4 yıl önce değişiklik isteğini kapattı
jonathan kris/refactor_redis_service dalı silindi 4 yıl önce
Değişiklik isteği 5d199cbd98 olarak birleştirildi.
Bu konuşmaya katılmak için oturum aç.
Değerlendirici yok
Etiket Yok
Kilometre Taşı Yok
Atanan Kişi Yok
2 Katılımcı
Bildirimler
Bitiş Tarihi

Bitiş tarihi atanmadı.

Bağımlılıklar

Bu çekme isteği henüz bir bağımlılık içermiyor.

Yükleniyor…
Henüz bir içerik yok.