如何将这个函数拆分以降低认知复杂度?

huangapple go评论79阅读模式
英文:

How can I split this function for low congnitive complexity?

问题

我对Java还不熟悉,这个noob函数一直困扰着我。我已经下载了SonarLint,然后它就抛出了这个问题 方法的认知复杂度不应太高。我知道它看起来很丑,但有没有人可以指出我如何重新格式化以实现DRY原则,同时又不会像SonarLint所提到的那样具有很高的复杂度。

@PostMapping("/adduser")
//	@PreAuthorize("hasRole('ADMIN')")
public ResponseEntity<MessageResponse> registerUser(@Valid @RequestBody SignupRequest signUpRequest) {
	/*
	 * 这个控制器根据所有用户实体创建新用户
	 * 
	 */
	if (dbValue) {
		if (repository.existsByUsername(signUpRequest.getUsername())) {
			return ResponseEntity
				.badRequest()
				.body(new MessageResponse("Error: Username is already taken!"));
		}

		if (repository.existsByEmail(signUpRequest.getEmail())) {
			return ResponseEntity
				.badRequest()
				.body(new MessageResponse("Error: Email is already in use!"));
		}

		// 创建新用户帐户
		User2 user = new User2(signUpRequest.getUsername(), 
							   signUpRequest.getEmail(),
							   signUpRequest.getCustomername(),
							   signUpRequest.getCustomerid(),
							   signUpRequest.getDescription(),
							   encoder.encode(signUpRequest.getPassword()));				
		
		Set<String> strRoles = signUpRequest.getRoles();				
		Set<Role2> roles = new HashSet<>();
		Role2 e = new Role2();
		e.setName("ROLE_ADMIN");
		roles.add(e);				
		user.setRoles(roles);
		repository.save(user);		
	} else {
		 
		if (repository.existsByUsername(signUpRequest.getUsername())) {
			return ResponseEntity
				.badRequest()
				.body(new MessageResponse("Error: Username is already taken!"));
		}

		if (repository.existsByEmail(signUpRequest.getEmail())) {
			return ResponseEntity
				.badRequest()
				.body(new MessageResponse("Error: Email is already in use!"));
		}

		// 创建新用户帐户
		User1 user = new User1(signUpRequest.getUsername(), 
							   signUpRequest.getEmail(),
							   signUpRequest.getCustomername(),
							   signUpRequest.getCustomerid(),
							   signUpRequest.getDescription(),
							   encoder.encode(signUpRequest.getPassword()));
		

		Set<String> strRoles = signUpRequest.getRoles();
		Set<Role> roles = new HashSet<>();
	      
		if (strRoles == null) {
			Role userRole = roleRepository1.findByName(URole.ROLE_USER)
				.orElseThrow(() -> new RuntimeException("Error: Role is not found."));
			roles.add(userRole);
		} else {
			strRoles.forEach(role -> {
				switch (role) {
				case "admin":
					Role adminRole = roleRepository1.findByName(URole.ROLE_ADMIN)
						.orElseThrow(() -> new RuntimeException("Error: Role is not found."));							
					roles.add(adminRole);
					break;

				default:
					Role userRole = roleRepository1.findByName(URole.ROLE_USER)
						.orElseThrow(() -> new RuntimeException("Error: Role is not found."));
					roles.add(userRole);
				}
			});
		}
		user.setRoles(roles);
		repository.save(user);
	}		
	return ResponseEntity.ok(new MessageResponse("User Added successfully!"));		
}

欢迎批评和任何帮助。提前感谢。

英文:

I'm new to Java and this noob function has been bothering me. I have downloaded sonarLint and lo and behold it throws this issue on my face Cognitive Complexity of methods should not be too high. I know it looks ugly but can anyone point out how I can reformat to have dry concept and also not have high complexity as mentioned by the SonarLint.

@PostMapping(&quot;/adduser&quot;)
//	@PreAuthorize(&quot;hasRole(&#39;ADMIN&#39;)&quot;)
public ResponseEntity&lt;MessageResponse&gt; registerUser(@Valid @RequestBody SignupRequest signUpRequest) {
/*
* This controller Creates new user based on all the entities for the user
* 
*/
if(dbValue)
{
if (repository.existsByUsername(signUpRequest.getUsername())) {
return ResponseEntity
.badRequest()
.body(new MessageResponse(&quot;Error: Username is already taken!&quot;));
}
if (repository.existsByEmail(signUpRequest.getEmail())) {
return ResponseEntity
.badRequest()
.body(new MessageResponse(&quot;Error: Email is already in use!&quot;));
}
// Create new user&#39;s account
User2 user = new User2(signUpRequest.getUsername(), 
signUpRequest.getEmail(),
signUpRequest.getCustomername(),
signUpRequest.getCustomerid(),
signUpRequest.getDescription(),
encoder.encode(signUpRequest.getPassword()));				
Set&lt;String&gt; strRoles = signUpRequest.getRoles();				
Set&lt;Role2&gt; roles = new HashSet&lt;&gt;();
Role2 e = new Role2();
e.setName(&quot;ROLE_ADMIN&quot;);
roles.add(e);				
user.setRoles(roles);
repository.save(user);		
}
else {
if (repository.existsByUsername(signUpRequest.getUsername())) {
return ResponseEntity
.badRequest()
.body(new MessageResponse(&quot;Error: Username is already taken!&quot;));
}
if (repository.existsByEmail(signUpRequest.getEmail())) {
return ResponseEntity
.badRequest()
.body(new MessageResponse(&quot;Error: Email is already in use!&quot;));
}
// Create new user&#39;s account
User1 user = new User1(signUpRequest.getUsername(), 
signUpRequest.getEmail(),
signUpRequest.getCustomername(),
signUpRequest.getCustomerid(),
signUpRequest.getDescription(),
encoder.encode(signUpRequest.getPassword()));
Set&lt;String&gt; strRoles = signUpRequest.getRoles();
Set&lt;Role&gt; roles = new HashSet&lt;&gt;();
if (strRoles == null) {
Role userRole = roleRepository1.findByName(URole.ROLE_USER)
.orElseThrow(() -&gt; new RuntimeException(&quot;Error: Role is not found.&quot;));
roles.add(userRole);
} else {
strRoles.forEach(role -&gt; {
switch (role) {
case &quot;admin&quot;:
Role adminRole = roleRepository1.findByName(URole.ROLE_ADMIN)
.orElseThrow(() -&gt; new RuntimeException(&quot;Error: Role is not found.&quot;));							
roles.add(adminRole);
break;
default:
Role userRole = roleRepository1.findByName(URole.ROLE_USER)
.orElseThrow(() -&gt; new RuntimeException(&quot;Error: Role is not found.&quot;));
roles.add(userRole);
}
});
}
user.setRoles(roles);
repository.save(user);
}		
return ResponseEntity.ok(new MessageResponse(&quot;User Added successfully!&quot;));		
}	

Appreciate criticism and any help. Thanks in Advance.

答案1

得分: 3

  1. 在控制器中,您需要与一个服务一起工作,而不是一个存储库。在那里创建用户创建方法。
  2. 创建一个名为UserValidator的接口,带有方法(existsByUsername、existsByEmail实现predicate)
Optional check(Predicate condition, String message)

或者将这些方法委托给一个新的验证工具类。
3. 您有重复的代码块,可以将其移出条件判断

if (repository.existsByUsername(signUpRequest.getUsername())) 
    return ResponseEntity
        .badRequest() // 下面,您总是在创建一个新的字符串和新的消息响应,请使用常量,以免堆积内存
        .body(new MessageResponse("Error: Username is already taken!"));
if (repository.existsByEmail(signUpRequest.getEmail())) 
    return ResponseEntity
        .badRequest() // 这里也一样
        .body(new MessageResponse("Error: Email is already in use!"));
  1. signUpRequest.getRoles() 可能会返回一个 Optional,或者如果为空,则返回 Collections.emptyList(),而不是 null。
  2. 通过 .valueOf() 获取角色,这样 .forEach() 条件和 switch-case 也就不再需要了。
  3. 将这部分代码
Role userRole = roleRepository1.findByName(URole.ROLE_USER)
   .orElseThrow(() -> new RuntimeException("Error: Role is not found."));
roles.add(userRole);

替换为

roleRepository1.findByName(URole.ROLE_USER).ifPresent(roles::add);
英文:
  1. In the controller, you need to work with a service, not a repository. Make user creation method there.
  2. Create interface UserValidator with method (existsByUsername, existsByEmail implements predicate)
    Optional check(Predicate condition, String message)
    OR delegate this methods to a new validation utility class.
  3. You have dublicate, may move it outside the condition
if (repository.existsByUsername(signUpRequest.getUsername())) 
return ResponseEntity
.badRequest() // below, you always create a new string and a new message response, use a constant as not to clog the memory
.body(new MessageResponse(&quot;Error: Username is already taken!&quot;));
if (repository.existsByEmail(signUpRequest.getEmail())) 
return ResponseEntity
.badRequest() // here too
.body(new MessageResponse(&quot;Error: Email is already in use!&quot;));
  1. signUpRequest.getRoles() may returns an Optional, or Collections.emptyList() if an empty, not a null
  2. Get role by .valueOf(), it make .forEach() conditions unused and switch-case too.
  3. Replace this
Role userRole = roleRepository1.findByName(URole.ROLE_USER)
.orElseThrow(() -&gt; new RuntimeException(&quot;Error: Role is not found.&quot;));
roles.add(userRole);

with

roleRepository1.findByName(URole.ROLE_USER).ifPresent(roles::add);

答案2

得分: 2

只需要始终使用 提取方法(Extract Method),这将极大地改善您的代码。这样做还将帮助您发现责任,并为进一步的重构铺平道路。

您可以将现有的代码保持原样,只需将其拆分为更小的自我描述方法,以使主算法更加易于理解。

例如:

if (repository.existsByUsername(signUpRequest.getUsername())) {
    return usernameTakenError();
}
if (repository.existsByEmail(signUpRequest.getEmail())) {
    return emailUsedError();
}
user = userToSignup(signUpRequest);
userRepository.save(user);
return ResponseEntity.ok(new MessageResponse("User Added successfully!"));

此时,这些方法的实际实现方式变得不那么重要:核心算法保持简短且易于理解。

请注意,我经常会自动将异常转换为响应错误,这允许以声明性方式检查规则,例如:

// 若已被占用则抛出异常,并由通用异常处理器返回错误响应
assertUsernameFree();

还要寻找DRY的机会。例如,不要写成:

roleRepository1.findByName(role).orElseThrow(() -> new RuntimeException("Error: Role is not found."));

可以使用 roleRepository1.roleOfName(role)findExistingByName,这些方法会抛出异常而不是返回 Optional

此外,您的代码中整个角色映射部分过于复杂,我认为其中可能存在错误:default 开关语句在其中将任何未知角色添加为 USER_ROLE,对我来说并没有什么实际意义。

我期望的是类似以下的代码(可能会位于 resolveSignupRoles 函数中,甚至是双重分派的 signUpRequest.getRoles(rolesRepo)):

Set<Role> roles = signUpRequest.getRoles().stream()
    .map(r -> URole.fromCode(r))
    .map(r -> userRepository::roleOfName).collect(Collectors.toSet());

if (roles.isEmpty()) roles.add(userRepository.roleOfName(URole.ROLE_USER));

这里存在大量的重构机会,但是 提取方法(Extract Method) 很容易应用,将立即改善您编写的所有代码。

英文:

Just look to apply Extract Method consistently and it will greatly improve your code. Doing so will also help you to discover responsibilities and pave the way for further refactoring.

You could take your existing code as is and just split it to smaller self-descriptive methods to make the main algorithm much more understandable.

E.g.

if (repository.existsByUsername(signUpRequest.getUsername())) {
return usernameTakenError();
}
if (repository.existsByEmail(signUpRequest.getEmail())) {
return emailUsedError();
}
user = userToSignup(signUpRequest);
userRepository.save(user);
return ResponseEntity.ok(new MessageResponse(&quot;User Added successfully!&quot;)); 

At this point it's much less important how those methods are actually implemented: the core algorithm remains short & understandable.

Note that I often convert exceptions to response errors automatically which allows to check rules in a declarative way
e.g.

//throws if taken and a generic exception handler 
//returns and error response
assertUsernameFree(); 

Also look for DRY opportunities. For instance, rather than:

roleRepository1.findByName(role).orElseThrow(() -&gt; new RuntimeException(&quot;Error: Role is not found.&quot;));

You could have roleRepository1.roleOfName(role) or findExistingByName which throws instead of returning Optional.

Furthermore, the whole roles mapping section of your code is way too complex and I think there might be a bug in there: the default switch case where any unknown role is added as USER_ROLE doesn't really seem to make any sense to me.

I'd expect something more along the lines of (which would be in a resolveSignupRoles function or even double-dispatch signUpRequest.getRoles(rolesRepo)):

Set&lt;Role&gt; roles = signUpRequest.getRoles().stream()
.map(r -&gt; URole.fromCode(r))
.map(r -&gt; userRepository::roleOfName).collect(Collectors.toSet());
if (roles.isEmpty()) roles.add(userRepository.roleOfName(URole.ROLE_USER));

There are tons of refactoring opportunities here, but Extract Method is easy to apply and will immediately improve all the code you write.

huangapple
  • 本文由 发表于 2020年10月8日 08:57:14
  • 转载请务必保留本文链接:https://go.coder-hub.com/64254318.html
匿名

发表评论

匿名网友

:?: :razz: :sad: :evil: :!: :smile: :oops: :grin: :eek: :shock: :???: :cool: :lol: :mad: :twisted: :roll: :wink: :idea: :arrow: :neutral: :cry: :mrgreen:

确定