刚入职的小白写了一个登录的接口，但感觉写的很臃肿，请教一下大佬有没有可以优化的地方呢？

[zx9481]

@Override
    public Response login(String params) {
        try {
            JSONObject jsonObject = JSON.parseObject(params);
            // 解密手机号码
            AesNewUtil aes = new AesNewUtil();
            String decMobile = aes.decrypt(jsonObject.getString("mobile"));
            // 获取 openId
            String openId = jsonObject.getString("openId");
            if (StringUtils.isEmpty(openId)) {
                return Response.fail(RespCode.PARAM_NULL);
            }
            logger.info("openId : {}", openId);
            // 查询代客用户表
            ValetUser valetUser = selectUserByMobile(decMobile);
            // 用户是否存在于白名单中
            if (null == valetUser) {
                return Response.fail(RespCode.NO_GET_INFO);
            } else {
                String userId = valetUser.getUserId();
                // 判断用户信息是否存在
                ValetUserInfo valetUserInfo = valetUserInfoMapper.selectByPrimaryKey(userId);
                if (null == valetUserInfo) {
                    // 保存用户信息
                    valetUserInfo = new ValetUserInfo();
                    valetUserInfo.setUserId(userId);
                    valetUserInfo.setOpenId(openId);
                    valetUserInfo.setCreateTime(DateUtils.getNowDate());
                    valetUserInfo.setUpdateTime(DateUtils.getNowDate());
                    valetUserInfoMapper.insertSelective(valetUserInfo);
                }
                // 获取 token
                String token = TokenUtil.generateToken(userId, decMobile, "app");
                cacheOpt.getOpt(Constants.CACHE_STR).opsForValue().set(Constants.USER_AUTH_PRIFIX + userId, token, 7, TimeUnit.DAYS);
//                cacheOpt.getOpt(Constants.CACHE_STR).opsForValue().set(Constants.LOGIN_MOBILE_AUTH_PRIFIX + userId, decMobile, 7, TimeUnit.DAYS);
//                logger.info((String) cacheOpt.getOpt(Constants.CACHE_STR).opsForValue().get(Constants.USER_AUTH_PRIFIX + valetUser.getUserId()));
                Map<String, Object> result = new HashMap<>();
                // 用户类型:0 无效，1 网格，2 客户经理，3 泛渠道，4 其他
                String userType = String.valueOf(valetUser.getUserType());
                if ("0".equals(userType)) {
                    userType = "无效";
                } else if ("1".equals(userType)) {
                    userType = "网格";
                } else if ("2".equals(userType)) {
                    userType = "客户经理";
                } else if ("3".equals(userType)) {
                    userType = "泛渠道";
                } else {
                    userType = "其他";
                }
                result.put("mobile", valetUser.getMobile());
                result.put("userName", valetUser.getUserName());
                result.put("cusName", valetUser.getCusName());
                result.put("userType", userType);
                result.put("operId", valetUser.getOperId());
                result.put("orgId", valetUser.getOrgId());
                result.put("merchantNum", valetUser.getMerchantNum());
                result.put("token", token);
                // 更新用户表登录时间
                valetUserMapper.updateLoginTimeByUserId(DateUtils.getNowDate(), userId);
                logger.info("登录成功 {}", decMobile);
                return Response.success("登录成功", result);
            }
        } catch (ValetappException e) {
            logger.error("登录失败 ： " + e.getMessage());
            return Response.fail(RespCode.FAIL);
        }
    }

没想到随便一发有这么多大佬ヾ(≧▽≦*)o 谢谢大家

[lcy630409]

写的很好，没问题。初学者 这样写很好的，能运行，让别人容易看懂就是好代码

[SmaliYu]

我觉得挺好，能用能看明白，别的都是锦上添花

[Rache1]

(⊙ˍ⊙)，Java 被你写成了 JavaScript 的感觉，Java 强类型的优势全无了。

接收请求参数应该创建一个类型实体来，而不是直接在 JSON 对象上获取，响应也是。

userType 判断那块，可以用 Map 来处理。「用户是否存在于白名单中」，你那里 return 了 下面就可以不要 else 了，下面直接写在那个 if 外就可以了。

[kosmosr]

返回不要用 map 封装 很难维护 最好写一个返回的 vo 然后 userType 可以用枚举封装一下

[356693212]

@Rache1 所以前端看懂了 哈哈

[sinnosong1]

//用户类型:0 无效，1 网格，2 客户经理，3 泛渠道，4 其他 这里可以建个 enum ，后面需要扩充的时候再修改

[Latin]

所以楼主就是那个小白吗

[chendy]

json 解析都进业务层了，数据校验还需要自己 try catch ，楼主不是钓鱼拉血压的吧……
还有这个奇怪的 override ，是用了啥奇怪的框架还是写了一个毫无意义的 service 接口……

[MoYi123]

1. valetUserInfo.setCreateTime(DateUtils.getNowDate()); valetUserInfo.setUpdateTime(DateUtils.getNowDate());
创建时间和更新时间应该自动加, 手动加容易忘记.
2. 不要注释代码, 哪天想找回来 git 里有记录的.

[ming159]

代码 "臃肿" 并不是问题. 个人看法是 耦合度太高.无法应对需求变动,建议参考其它用户,权限,角色管理系统. 我个人认为存在的问题有这么几个
1. login 方法中的参数,未做比较严格的校验,如果客户端传入了 10 万字符长度的,你这里会不会卡?
2. 是否可以将参数校验单独提取为一个 方法? validator
3. 用户黑白名单与角色的功能是写死的,如果需求变动,是需要改代码的. 如果允许可以使用 apache shiro 这种成熟的权限框架,如果觉得重,也务必将 授权,鉴权 独立为两个模块,方便扩展.

[hidemyself]

valatUserInfo,userType 可以抽成一个函数
尽量不要用 map
最后的组装可以用 builder 或者 Beanutil 这种东西

当然以上都是吹毛求疵了，我个人会做成这样

[LLaMA2]

private final MemberService memberService;
public ResponseEntity<?> login(@NotNull final String mobile, @NotNull final String passwd) {
String decrptMobile = AesUtil.decrypt(mobile);
Member member = memberService.findByMobile(mobile);
if(BCrypt.check(passwd, member.passwd)) throw new LoginException('登录失败');
String token = TokenUtil.generate(decMobile);
LoginResp resp = LoginResp.builder()
.name(member.name)
.age(member.age)
.token(token)
.build();
ResponseEntity.status(HttpStatus.OK)
.header("token", token)
.body(resp);
}

[storyxc]

1. 不要用 Map 做方法参数 /返回值，建单独的 param/vo 类，否则以后维护接口的时候看到这么长一串参数会一脸懵逼
2. 代码里不要写 0 ，1 ，2 ，3 ，4 。。。这种魔法数字，建立常量 /枚举来维护这些值

[janus77]

初学这样写挺好的，后面就是抽离封装方法了。可以重复利用的功能抽成一个方法，比如白名单，用户存在，用户的类型，参数的 put ，用户登录的更新，这些抽成方法，然后去调用。
再进一步就要用到架构类的知识了，现在这样就行

[bojue]

{'0':'无效'}[userType]

[pengtdyd]

过来人的经验：能跑就行！！！

[KK7890]

看到了 论坛的大爱。。。。谢谢大家 虽然我看不懂

[rootx]

“又不是不能用”🌸🐔

[zx9481]

@Rache1 请问 userType 怎么用 map 处理呢？有点不太理解。

[siweipancc]

按我们组长的话“能用，不慢就行”

[SingeeKing]

写的很棒，毫无 Java 感（

[jalena]

util 类为啥不是 static ！！！还要 new ？

再者入参？ java 难道不支持 entity ？

再说了，放开胆子干啊！！！ JSONObject 啊

[jalena]

要写 java ，了解下 Lombok 。。。。。。声明个类，加个 @data ，分分钟的事情。。要想骚，再加个 @build

[jalena]

顺便再了解下 JSR303 ！

你这登录就妥了~

[jalena]

顺便再夸奖下，比我当年写的好！！

[daimubai]

小白也提出一点建议哈，
1. 首先 Java 是一个面向对象的语言，全程 json 、map 这这这，参数和返回结果自然要用一个对象来处理
2. 既然 return 了，那这里的 else 没有必要，我个人不喜欢把一堆代码包在 else 中
3. 取 userType 这里我会用枚举 switch enum ，并且单独抽出一个方法 getUserType()

[daimubai]

而且使用对象的话，你可以用到很多的插件，比如一键生成属性的 setter 方法啊等等

[Tenlearn]

上来就报个 NPE 你怕不怕？

[season8]

优化是吧？
你这个方法真的很长，层次也很深，做的事也很多，显得很略复杂。


1. 入参用 string 、map 都是增加后期维护成本，复杂方法都不知道具体属性
2. 这个 try catch 粗暴，可以的话在具体业务方法上加
3. 你这个 StringUtils.isEmpty(openId) ， 要是传的 空串咋办
4. 手机号也没做非空校验，当然可能解密会抛异常，但这样不够直观
5. if(xxx){return} else{xxx}这种写法 去掉 else 能减少代码层次，稍微提高可读性
6. 保存用户信息可以提出来
7. userType 转换 也可以提出来，用 枚举、map 或者 直接提出来一个简单的方法都可以
8. 最后返回的 result 几乎就是把 valetUser 属性拿了一遍，能不能用 JSON.toJSON(valetUser)，或者 Response 设计成泛型，直接放入 valetUser
9. 个人觉得好多注释都没必要，大多是一眼就能看出来是干啥的，你可以在关键业务或者复杂上加，或者用 log 代替 注释

[h1104350235]

这个有点像我写 Node

[season8]

@season8 关于第 3 点，我是当你用的 commons-lang3.StringUtils ，是的话用 isNotBlank 代替

[Rache1]

@zx9481 😂 建议使用枚举了，写这个评论的时候忘了枚举这茬儿了。

[9c04C5dO01Sw5DNL]

人生就这样，凑合过吧（狗头

[night98]

槽点太多，哈哈

@Override
public Response login(String params) { // 入参改成 VO
try {
JSONObject jsonObject = JSON.parseObject(params); // 这块干掉
// 解密手机号码
AesNewUtil aes = new AesNewUtil();
String decMobile = aes.decrypt(jsonObject.getString("mobile")); // 改成静态方法
// 获取 openId
String openId = jsonObject.getString("openId");
if (StringUtils.isEmpty(openId)) { // 入参时校验
return Response.fail(RespCode.PARAM_NULL);
}
logger.info("openId : {}", openId);
// 查询代客用户表
ValetUser valetUser = selectUserByMobile(decMobile);
// 用户是否存在于白名单中
if (null == valetUser) { // 入参时校验，了解一下自定义注解验证
return Response.fail(RespCode.NO_GET_INFO);
} else {
String userId = valetUser.getUserId();
// 判断用户信息是否存在
ValetUserInfo valetUserInfo = valetUserInfoMapper.selectByPrimaryKey(userId);
if (null == valetUserInfo) {
// 保存用户信息
valetUserInfo = new ValetUserInfo();
valetUserInfo.setUserId(userId);
valetUserInfo.setOpenId(openId);
valetUserInfo.setCreateTime(DateUtils.getNowDate());
valetUserInfo.setUpdateTime(DateUtils.getNowDate());
valetUserInfoMapper.insertSelective(valetUserInfo);
}
// 获取 token
String token = TokenUtil.generateToken(userId, decMobile, "app");
cacheOpt.getOpt(Constants.CACHE_STR).opsForValue().set(Constants.USER_AUTH_PRIFIX + userId, token, 7, TimeUnit.DAYS);
// cacheOpt.getOpt(Constants.CACHE_STR).opsForValue().set(Constants.LOGIN_MOBILE_AUTH_PRIFIX + userId, decMobile, 7, TimeUnit.DAYS);
// logger.info((String) cacheOpt.getOpt(Constants.CACHE_STR).opsForValue().get(Constants.USER_AUTH_PRIFIX + valetUser.getUserId()));
Map<String, Object> result = new HashMap<>(); //返回改成 dto
// 用户类型:0 无效，1 网格，2 客户经理，3 泛渠道，4 其他
String userType = String.valueOf(valetUser.getUserType()); // 这里改成枚举+find 方法
if ("0".equals(userType)) {
userType = "无效";
} else if ("1".equals(userType)) {
userType = "网格";
} else if ("2".equals(userType)) {
userType = "客户经理";
} else if ("3".equals(userType)) {
userType = "泛渠道";
} else {
userType = "其他";
}
result.put("mobile", valetUser.getMobile()); // 这里用 beanutil 或者 mapstruct 优化，一行完事
result.put("userName", valetUser.getUserName());
result.put("cusName", valetUser.getCusName());
result.put("userType", userType);
result.put("operId", valetUser.getOperId());
result.put("orgId", valetUser.getOrgId());
result.put("merchantNum", valetUser.getMerchantNum());
result.put("token", token);
// 更新用户表登录时间
valetUserMapper.updateLoginTimeByUserId(DateUtils.getNowDate(), userId);
logger.info("登录成功 {}", decMobile);
return Response.success("登录成功", result);
}
} catch (ValetappException e) { //改成全局异常处理
logger.error("登录失败 ： " + e.getMessage());
return Response.fail(RespCode.FAIL);
}
}



改完差不多这样
@Override
public Response login(@Valid UserVO user) {
logger.info("openId : {}", openId);
String decMobile = AesNewUtil.decrypt(user.getMobile());
String userId = valetUser.getUserId();
ValetUser valetUser = selectUserByMobile(decMobile);
// 判断用户信息是否存在
ValetUserInfo valetUserInfo = valetUserInfoMapper.selectByPrimaryKey(userId);
if (Objects.isNull(valetUserInfo)) {
// 保存用户信息
valetUserInfo = ValetUserInfoMapper.convert(user);
valetUserInfoMapper.insertSelective(valetUserInfo);
}
String token = TokenUtil.generateToken(userId, decMobile, "app");
cacheOpt.getOpt(Constants.CACHE_STR).opsForValue().set(Constants.USER_AUTH_PRIFIX + userId, token, 7, TimeUnit.DAYS);

String userType = UserType.find(valetUser.getUserType());
TokenInfo token = tokenmapper.convert(valetUser,userType);
valetUserMapper.updateLoginTimeByUserId(DateUtils.getNowDate(), userId);
logger.info("登录成功 {}", decMobile);
return Response.success("登录成功", token);
}

[Joker123456789]

用户类型可以用枚举，不需要 if

最后的那个 map 是 多余的，直接返回 valetUser 即可

还有一个小建议，手机号不需要加密，实际应用中 配上 https 然后不要用 get 传输 就可以解决问题

[rehoni]

建议下个 alibaba coding guideline 插件，他会提示你一些修改的，比如魔法值，比如方法过长等等，已经很可以了，后边学的越多就写的越好了。