#11 Cleaning redis service and adding ttl getter method

병합
jonathan kris/refactor_redis_service 에서 master 로 4 commits 를 머지했습니다 4 년 전
kris 코멘트됨, 4 년 전
아직 콘텐츠가 없습니다.
kris changed title from WIP: (testing) Cleaning redis service to Cleaning redis service adn adding ttl getter method 4 년 전
kris 코멘트됨, 4 년 전
포스터

@jonathan please review

@jonathan please review
jonathan 다음으로부터 할당됨 kris 4 년 전
kris changed title from Cleaning redis service adn adding ttl getter method to Cleaning redis service and adding ttl getter method 4 년 전
jonathan 코멘트됨, 4 년 전
소유자

@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 년 전
포스터
  • 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 closed this pull request 4 년 전
jonathan 삭제된 브랜치 kris/refactor_redis_service 4 년 전
The pull request has been merged as 5d199cbd98.
로그인하여 이 대화에 참여
No reviewers
레이블 없음
마일스톤 없음
담당자 없음
참여자 2명
알림
마감일

마감일이 설정되지 않았습니다.

의존성

이 풀 리퀘스트는 어떠한 의존성도 가지지 않습니다.

불러오는 중...
아직 콘텐츠가 없습니다.