narendly commented on issue #357: [WIP] Add getWorkflows(long timeout) to TaskDriver. URL: https://github.com/apache/helix/pull/357#issuecomment-514293990 I think Optional would be beneficial if you want to have fluid chained-calls (similar to functional programming). Here, it is possible to avoid optionality/returning null by checking for the presence of a field. So using Optional is not necessary because you do not need to introduce null in the first place. It would also incur performance/memory overhead. -Hunter On Jul 23, 2019, 18:45, at 18:45, pkuwm <notificati...@github.com> wrote: >For the isWFConfig part, I understand. Definitely doable. > >Just for a discussion about Optional. I think it is designed to solve >the >problem of NullPointerException/null smell. As isWFConfig is like half >code >of fromHelixProperty, using Optional may seem cleaner to me :-) > >On Tue, Jul 23, 2019 at 2:37 AM Hunter Lee <notificati...@github.com> >wrote: > >> I don't think using Optional just for the sake of removing null is >> appropriate. I wouldn't think it would help with performance either. >You >> could call isWorkflowConfig and parse if it is true, and skip if >false, >> thereby doing minimal amount of parsing needed overall. >> >> On Jul 23, 2019, 11:26, at 11:26, pkuwm <notificati...@github.com> >wrote: >> >> As for parseWorkflow - I think it will be cleaner if we could >have >> >something like isWorkflowConfig() instead that returns T/F, and >based >> >on the result, include it here or not. This way, we do not return >null >> >(a code smell), and there are other places in the codebase where we >> >could re-use isWorkflowConfig. >> >> […](#) >> >> On Jul 23, 2019, 10:02, at 10:02, pkuwm ***@***.***> wrote: >Wonder >> >what the unit test requirement is. Can you please suggest? -- You >are >> >receiving this because you are subscribed to this thread. Reply to >this >> >email directly or view it on GitHub: [#357 >> >(comment)]( >> https://github.com/apache/helix/pull/357#issuecomment-514099797) >> > >> >OK. If null is not welcomed (everyone hates null), how about >Optional >> >to avoid null? I personally like Optional more than `null` or >ignore >> >exceptions in a catch block. Do you guys like Optional to possibly >> >handle the `null` cases? >> > >> >> As for parseWorkflow - I think it will be cleaner if we could >have >> >something like isWorkflowConfig() instead that returns T/F, and >based >> >on the result, include it here or not. This way, we do not return >null >> >(a code smell), and there are other places in the codebase where we >> >could re-use isWorkflowConfig. >> >> […](#) >> >> On Jul 23, 2019, 10:02, at 10:02, pkuwm ***@***.***> wrote: >Wonder >> >what the unit test requirement is. Can you please suggest? -- You >are >> >receiving this because you are subscribed to this thread. Reply to >this >> >email directly or view it on GitHub: [#357 >> >(comment)]( >> https://github.com/apache/helix/pull/357#issuecomment-514099797) >> > >> > >> > >> >-- >> >You are receiving this because you commented. >> >Reply to this email directly or view it on GitHub: >> >https://github.com/apache/helix/pull/357#issuecomment-514130185 >> >> — >> You are receiving this because you authored the thread. >> Reply to this email directly, view it on GitHub >> ><https://github.com/apache/helix/pull/357?email_source=notifications&email_token=ABHSRCOPOLLEO6EL7AZIYY3QA3GMRA5CNFSM4IGBGEJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2SQ72A#issuecomment-514133992>, >> or mute the thread >> ><https://github.com/notifications/unsubscribe-auth/ABHSRCLANEKNRZXDHCPLNCTQA3GMRANCNFSM4IGBGEJA> >> . >> > > >-- >You are receiving this because you commented. >Reply to this email directly or view it on GitHub: >https://github.com/apache/helix/pull/357#issuecomment-514291290
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
