caicancai commented on PR #3481: URL: https://github.com/apache/incubator-streampark/pull/3481#issuecomment-1890384610
> > Hi, @zzzk1 Thanks for your contribution and for @caicancai review. > > To be honest, I'm more inclined towards the scope of responsibilities in this [proposal-1](https://streampark.apache.org/community/submit_guide/code_style_and_quality_guide), so I would prefer to discard changes that are not within the scope of the proposal in the current PR. > > Of course, we'd be glad to consider accepting changes beyond the scope, but we need to make this decision through some necessary discussions. > > So, in short, if you want to quickly merge this PR, you can remove changes that are not within the proposal scope; If you wish to merge all the contents of this PR, we look forward to you initiating this discussion to determine whether new rules can be introduced. Please relax, this does not mean that additional changes are incorrect, it just requires a community oriented discussion process. > > Please let me know what's your opinion~ > > Hi @RocMarshal , I think the following coding rules can be introduced > > * If the argument passed by the front end is single, the **Controller** uses that single argument to receive without using object wrappers > > * If the **Controller** passes an object to the **Service**, and only a single field in the object is used in the Service, the object is replaced with a single parameter > ```java > /* before */ > /* controller */ > public RestResponse delete(Member member) { > this.memberService.remove(member); > return RestResponse.success(); > } > /* service */ > public void remove(Member memberArg) { > Member member = > Optional.ofNullable(this.getById(memberArg.getId())) > .orElseThrow( > () -> > new ApiAlertException( > String.format("The member [id=%s] not found", memberArg.getId()))); > this.removeById(member); > userService.clearLastTeam(member.getUserId(), member.getTeamId()); > } > > > /* after */ > /* controller */ > public RestResponse delete(Long id) { > this.memberService.remove(id); > return RestResponse.success(); > } > > /* service */ > public void remove(Long id) { > Member member = > Optional.ofNullable(this.getById(id)) > .orElseThrow( > () -> new ApiAlertException(String.format("The member [id=%s] not found", id))); > this.removeById(member); > userService.clearLastTeam(member.getUserId(), member.getTeamId()); > } > ``` > > * Use distinct and well-defined parameter names in **Service** to avoid ambiguity > ```java > /* Bad case */ > List<ApplicationConfig> list(Long id); > > /* good case */ > List<ApplicationConfig> list(Long appId); > ``` Here I think we can’t just look at the current interface design of sp. We must consider the scalability of the sp controller interface. You have to ensure that other parameters in the entire object will not be used in the future. I have to say that sp still has a lot of potential here. There is room for improvement. I cannot guarantee that the parameters in the object will not be used in the future. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
