-
Notifications
You must be signed in to change notification settings - Fork 254
改进部分代码细节: #80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
改进部分代码细节: #80
Conversation
|
能否详细说明改动了哪些部分,以及为什么要这样做吗? @CodePlayer |
| //上次更新时间 | ||
| private volatile Instant instant; | ||
|
|
||
| protected volatile Long lastUpdate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
为什么要用Long替代Instant ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xy-peng 不好意思,我其实最初只打算 Pull Request 第一个 commit 的。
我提交了第一个 commit 后就创建了该 Pull Request ,之后我才继续进行的后面两次 commit 的代码修改。
由于后面两次 commit 改动的地方太多,一般的项目 Owner 有可能忌讳这种大幅改动,所以我并没有打算全部 Pull Request,只是提交到了自己的 fork,但不知为何,Github 也将其自动合并到了该 Pull Request 中。
不过,既然已经提交了,那我就简单解释说明一下:
- 我建议将项目中用到的请求头统一在
Headers中以常量的形式定义起来。这样避免多次引用的时候因手误敲错字符而导致不一致。并且,这样也方便开发者在项目中直接引用官方定义好的常量,避免手误。 - 使用 Java 7 中内置的 UTF-8 CharSet 替代 字符串字面值参数,避免不必要的 IOE try-catch(我看了下,官方项目本身是基于 Java 8 的,因此可以安全替换,没有JDK兼容性问题)。
- 使用 Java 7 新的 try-with-resources 语法糖 简化 try-finally-close 语句。
- 为什么要用 Long 替代 Instant ? 因为,我看了下这段代码的关键作用就是判断证书的更新时间,以决定是否进行自动更新。如果使用
Instant和Duration来判断,虽然也能实现该目的,但是生成的中间对象较多,执行的代码也比较多,执行效率也就相对低一点(其实,相对整个项目来说,这一点额外的开销几乎可以忽略不计,但是蚊子肉也是肉,可以省一点也好)。 TimeInterval枚举我也去掉了,因为我个人认为 JDK 自带的TimeUnit枚举可以起到相同的作用。- 我发现
PemUtil.loadPrivateKey( InputStream )这个方法只在单元测试中用到了,单元测试里面先把字符串通过String.getBytes()转为byte[],再进而将其转为ByteArrayInputStream,然后传入到上述方法中。结果,又在该方法中通过ByteArrayOutputStream转为byte[],再通过new String()恢复为原字符串。这样做实际上是绕了一圈,所以我就添加了一个PemUtil.loadPrivateKey( String )的重载方法。 WechatPayHttpClientBuilder.withWechatpay()方法的字母大小写和项目中其他代码的命名风格有冲突,所以我就改为了 withWechatPay。但是,我又担心 SDK 的向后兼容性问题,所以我又将原命名的方法加回来了,标记为@Deprecated,并且加了 Javadoc 注释。- 实际上要保证SDK的向后兼容性,TimeInterval 枚举也不应该去掉。不过,就算开发者在项目代码中确实使用了这些 SDK API ,在替换成新版本时,也会直接编译不通过,从而警示用户修改。 如果要优先保证SDK 的向后兼容性,那么请保留 TimeInterval 枚举。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 还有就是,我把代码的格式作了些许规范性调整,使代码风格看起来更加统一(第一次 commit 中我并没有改动任何代码格式,只是在后面两次本来只打算提交到我自己的 fork 中的代码作了格式化)。另外,我看到原来的代码缩进用的是两个空格,所以我也沿用了原代码的这种缩进风格。
- 如果对于代码中的某些地方还有疑问,你可以直接在对应的代码处 @我,我这边也都可以尽快回复并加以解释说明。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
感谢贡献,感谢周末写下的大段文字,让我们能更好的理解你的想法。
同意你几乎所有的观点,除了Java.Instant和Long的替换 😄 虽然性能上有理论上的提升,但是损失了代码的可读性。
Duration.between(instant, Instant.now()).toMinutes() >= minutesInterval
System.currentTimeMillis() - lastUpdate >= minutesInterval * 1000L * 60两者相比,我还是倾向于使用Java.Instant,直观易懂(当然其实大部分人都懂timestamp),也降低了出错的概率。
| @@ -0,0 +1,21 @@ | |||
| package com.wechat.pay.contrib.apache.httpclient; | |||
|
|
|||
| public interface Headers { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这些常量的定义,为什么不放到util包里边,而做成一个interface接口?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这应该属于个人偏好问题,就像 Spring 习惯将工具类定义为 abstract 一样。而 Apache 为了兼容 Bean instance invoke 而倾向于不加 abstract。
常量的定义一般都是 public static final 的,而接口里面定义的变量天生就是常量,而且不需要写多余的 public static final 修饰符。所以,个人认为将常量放置在接口中定义看起来会更简洁更优雅。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
但是,这里并没有headers这样的一个接口,这里只是常量,跟原本要表达的含义就不一样了。
src/main/java/com/wechat/pay/contrib/apache/httpclient/SignatureExec.java
Show resolved
Hide resolved
src/main/java/com/wechat/pay/contrib/apache/httpclient/SignatureExec.java
Show resolved
Hide resolved
src/main/java/com/wechat/pay/contrib/apache/httpclient/SignatureExec.java
Show resolved
Hide resolved
src/main/java/com/wechat/pay/contrib/apache/httpclient/SignatureExec.java
Show resolved
Hide resolved
src/main/java/com/wechat/pay/contrib/apache/httpclient/auth/AutoUpdateCertificatesVerifier.java
Show resolved
Hide resolved
| public AutoUpdateCertificatesVerifier(Credentials credentials, byte[] apiV3Key, | ||
| int minutesInterval) { | ||
| public AutoUpdateCertificatesVerifier(Credentials credentials, byte[] apiV3Key, int minutesInterval) { | ||
| this.credentials = credentials; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
建议把 参数 minutesInterval 直接改成 long 型,TimeUnit 的 toMinutes 返回的本身是long,没必要再强制转成int,且 long 也是兼容 int的,不存在向后兼容的问题。
src/main/java/com/wechat/pay/contrib/apache/httpclient/auth/WechatPay2Validator.java
Show resolved
Hide resolved
| timestamp.getValue(), requestId); | ||
| String timestampStr = header.getValue(); | ||
| try { | ||
| long timestampInSecond = Long.parseLong(timestampStr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
时间相关操作改为面向对象的写法
|
|
||
| public static final String TRANSFORMATION = "AES/GCM/NoPadding"; | ||
|
|
||
| static final int KEY_LENGTH_BYTE = 32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
建议统一加上访问修饰符
src/test/java/com/wechat/pay/contrib/apache/httpclient/AutoUpdateVerifierTest.java
Show resolved
Hide resolved
src/test/java/com/wechat/pay/contrib/apache/httpclient/HttpClientBuilderTest.java
Show resolved
Hide resolved
|
考虑到规范和编程习惯的分歧,我们会在你贡献的基础上进行修改,再重新发起 PR #85 。感谢你的贡献 @CodePlayer ,未来也希望你能多多参与。 |
使用 UTF-8 Charset 替代 字符串;
使用 String.replace 替代 部分 String.replaceAll (避免正则表达式的额外开销);
使用 Java 7、8 的语法简化部分代码。