{"action":"created","issue":{"url":"https://api.github.com/repos/ryanhoo/fir.flight/issues/1","repository_url":"https://api.github.com/repos/ryanhoo/fir.flight","labels_url":"https://api.github.com/repos/ryanhoo/fir.flight/issues/1/labels{/name}","comments_url":"https://api.github.com/repos/ryanhoo/fir.flight/issues/1/comments","events_url":"https://api.github.com/repos/ryanhoo/fir.flight/issues/1/events","html_url":"https://github.com/ryanhoo/fir.flight/issues/1","id":170369236,"number":1,"title":"几点建议","user":{"login":"Piasy","id":3098704,"avatar_url":"https://avatars.githubusercontent.com/u/3098704?v=3","gravatar_id":"","url":"https://api.github.com/users/Piasy","html_url":"https://github.com/Piasy","followers_url":"https://api.github.com/users/Piasy/followers","following_url":"https://api.github.com/users/Piasy/following{/other_user}","gists_url":"https://api.github.com/users/Piasy/gists{/gist_id}","starred_url":"https://api.github.com/users/Piasy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Piasy/subscriptions","organizations_url":"https://api.github.com/users/Piasy/orgs","repos_url":"https://api.github.com/users/Piasy/repos","events_url":"https://api.github.com/users/Piasy/events{/privacy}","received_events_url":"https://api.github.com/users/Piasy/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2016-08-10T09:13:37Z","updated_at":"2016-08-10T10:01:14Z","closed_at":null,"body":"+ fabric 的 api key 没有 ignore\r\n+ 依赖声明有多处没有指明版本(有 + 号)\r\n+ app/develop 目录是干嘛的?\r\n+ 是否可以整理一个架构 wiki?\r\n+ [都 finish 了, 还 startActivityForResult 干嘛?](https://github.com/ryanhoo/fir.flight/blob/develop/app/src/main/java/io/github/ryanhoo/firFlight/ui/SplashScreenActivity.java#L107)\r\n+ [为什么不是在 animation 结束的时候做跳转, 而是延迟固定的时间呢? 要是动画时长改了, 这里是不是也就要改了?](https://github.com/ryanhoo/fir.flight/blob/develop/app/src/main/java/io/github/ryanhoo/firFlight/ui/SplashScreenActivity.java#L58)\r\n+ [AndroidStudio 2.2 beta 此处有两个 lint 提示, 好像有道理](https://github.com/ryanhoo/fir.flight/blob/develop/app/src/main/java/io/github/ryanhoo/firFlight/account/UserSession.java#L30)\r\n+ [DCL 单例没用 volatile](https://github.com/ryanhoo/fir.flight/blob/develop/app/src/main/java/io/github/ryanhoo/firFlight/account/UserSession.java#L49)\r\n+ [关于 if 语句是否需要花括号的讨论, 有一点感觉有道理: 如果你需要扩展为多行语句, 要么有花括号, 要么单行, 否则容易疏忽出错](https://github.com/ryanhoo/fir.flight/blob/develop/app/src/main/java/io/github/ryanhoo/firFlight/ui/signin/SignInActivity.java#L120)\r\n+ [此处有 lint 提示](https://github.com/ryanhoo/fir.flight/blob/develop/app/src/main/java/io/github/ryanhoo/firFlight/ui/signin/SignInActivity.java#L124), 建议升级 IDE, 并且对 lint 提示保持关注\r\n+ [RxJava 避免内存泄漏](https://github.com/ryanhoo/fir.flight/blob/develop/app/src/main/java/io/github/ryanhoo/firFlight/ui/signin/SignInActivity.java#L133)\r\n+ [像这个这么复杂的逻辑, 就值得写个单元测试了](https://github.com/ryanhoo/fir.flight/blob/develop/app/src/main/java/io/github/ryanhoo/firFlight/account/UserSession.java#L68)\r\n+ 可以看看 MVP 模式?\r\n+ 可以考虑下依赖注入? 虽然你也有一个 `Injection` 类\r\n+ 为什么要有 `RemoteTokenDataSource` 这个类? 为了扩展 `TokenContract.Remote` (提供超出 API 范围的 data 接口)? 或者对 API 请求做些控制? 如果是前者, 我觉得 `TokenRepository` 做这个事情还挺合适, 如果是后者倒还是可以理解\r\n+ 另外, 有必要抽象一个 `TokenRepository` 接口吗? 虽说面向接口是很好的实践, 但如果不会有其他的实现, 接口确实增加了负担\r\n\r\n\r\n## 最后有个问题想要一起讨论下\r\n\r\n+ 获取到新的 remote token 之后, 刷新 local token, `TokenRepository` 和 `UserSession`, 这件事谁做更合适?\r\n+ 更抽象一点，presenter 和 model，是否应该 model 提供基本、通用、业务无关的接口，由 presenter 利用业务无关接口实现业务相关需求？好吧，刚写出来我就觉得答案应该是肯定的。\r\n"},"comment":{"url":"https://api.github.com/repos/ryanhoo/fir.flight/issues/comments/238821519","html_url":"https://github.com/ryanhoo/fir.flight/issues/1#issuecomment-238821519","issue_url":"https://api.github.com/repos/ryanhoo/fir.flight/issues/1","id":238821519,"user":{"login":"ryanhoo","id":1622320,"avatar_url":"https://avatars.githubusercontent.com/u/1622320?v=3","gravatar_id":"","url":"https://api.github.com/users/ryanhoo","html_url":"https://github.com/ryanhoo","followers_url":"https://api.github.com/users/ryanhoo/followers","following_url":"https://api.github.com/users/ryanhoo/following{/other_user}","gists_url":"https://api.github.com/users/ryanhoo/gists{/gist_id}","starred_url":"https://api.github.com/users/ryanhoo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ryanhoo/subscriptions","organizations_url":"https://api.github.com/users/ryanhoo/orgs","repos_url":"https://api.github.com/users/ryanhoo/repos","events_url":"https://api.github.com/users/ryanhoo/events{/privacy}","received_events_url":"https://api.github.com/users/ryanhoo/received_events","type":"User","site_admin":false},"created_at":"2016-08-10T10:01:14Z","updated_at":"2016-08-10T10:01:14Z","body":"@Piasy 首先非常感谢这么用心的 review。\r\n\r\n抱歉，因为处于内测的阶段，代码没有完全整理完毕，有几个是遗留问题，或者疏忽了。\r\n\r\n- fabric 的 api key 没有 ignore\r\n\r\n疏忽，待改正。\r\n\r\n- 都 finish 了, 还 startActivityForResult 干嘛?\r\n\r\n老版本留下来的，那个时候没有用 RxBus，用的是传统的方式判断是否登录成功。\r\n\r\n- 依赖声明有多处没有指明版本(有 + 号)\r\n\r\n理论上是不推荐用 + 号，防止版本变更，老版本 API 表现不一致或者 API 废弃，导致编译失败。\r\n而我用 + 号是因为，**一般来说，版本号大概是这样 1.0.0，第三个版本一般是用来表达 bug 修复之类的变更，如果有功能、API 变更，应该反应在版本号的前两位上。**\r\n当然这么做值得商榷。\r\n\r\n- app/develop 目录是干嘛的?\r\n\r\n渠道专属目录，默认的渠道是 develop，我为这个渠道专门绘制了图标，而且使用不一样的 `application id`，目的是方便我区分测试和线上的应用。\r\n\r\n- 是否可以整理一个架构 wiki?\r\n\r\n可以，我还有计划就是为这个项目写一系列博客，本来就是技术的实验田，有很多值得分享、讨论的话题。\r\n\r\n- 为什么不是在 animation 结束的时候做跳转, 而是延迟固定的时间呢? 要是动画时长改了, 这里是不是也就要改了?\r\n\r\n历史遗留问题，待解决。\r\n\r\n- AndroidStudio 2.2 beta 此处有两个 lint 提示, 好像有道理\r\n\r\n有道理\r\n\r\n- DCL 单例没用 volatile\r\n\r\n补上，原来的单例不完全正确。\r\n\r\n- 关于 if 语句是否需要花括号的讨论, 有一点感觉有道理: 如果你需要扩展为多行语句, 要么有花括号, 要么单行, 否则容易疏忽出错\r\n\r\n因为懒而且确认只有一行代码，所以写了 [Yoda Condition](https://en.wikipedia.org/wiki/Yoda_conditions)，应该一直有花括号。\r\n\r\n- 此处有 lint 提示, 建议升级 IDE, 并且对 lint 提示保持关注\r\n\r\n有理\r\n\r\n- RxJava 避免内存泄漏\r\n\r\n漏了，用 MVP 重构的时候补上。\r\n\r\n- 像这个这么复杂的逻辑, 就值得写个单元测试了\r\n\r\n单元测试现在完全没有，没什么经验，希望你多多指导。\r\n\r\n- 可以看看 MVP 模式?\r\n\r\n部分已经用 MVP 重构完了，这里还没有（内测嘛，嘿嘿嘿）。\r\n\r\n- 可以考虑下依赖注入? 虽然你也有一个 Injection 类\r\n\r\n计划用 Dagger 2，关注 README 的 [Todo List](https://github.com/ryanhoo/fir.flight#to-do-list)，**项目早期特意采用比较原始的方式实现，然后一步步重构来优化代码，目的是为了感受代码改变带来的变化，以及各种方式的优劣，另外还有一个目的就是为了做 demonstration。**\r\n\r\n- 为什么要有 RemoteTokenDataSource 这个类? 为了扩展 TokenContract.Remote (提供超出 API 范围的 data 接口)? 或者对 API 请求做些控制? 如果是前者, 我觉得 TokenRepository 做这个事情还挺合适, 如果是后者倒还是可以理解，另外, 有必要抽象一个 TokenRepository 接口吗? 虽说面向接口是很好的实践, 但如果不会有其他的实现, 接口确实增加了负担。\r\n\r\n这个跟下面两个问题一样，处于大问号的阶段，需要寻找改进的方法。\r\n\r\n> 获取到新的 remote token 之后, 刷新 local token, TokenRepository 和 UserSession, 这件事谁做更合适?\r\n> 更抽象一点，presenter 和 model，是否应该 model 提供基本、通用、业务无关的接口，由 presenter 利用业务无关接口实现业务相关需求？好吧，刚写出来我就觉得答案应该是肯定的。"}}
{"actor":{"display_login":"ryanhoo"}}