freedomjs / freedom-port-control

Opens ports through a NAT with NAT-PMP, PCP, and UPnP
11 stars 3 forks source link

Code review #1

Closed kennysong closed 9 years ago

kennysong commented 9 years ago

Refactored significantly for code review.

Review on Reviewable

mollyling commented 9 years ago

LGTM, small comments inline.


Review status: 0 of 8 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


src/nat-pmp.js, line 233 [r1] (raw file): Btw, the array buffer's pre-initialized to zero, so these zero-valued rows are unnecessary.


src/pcp.js, line 255 [r1] (raw file): As above, these zero-valued rows aren't needed.


src/utils.js, line 114 [r1] (raw file): Two small things:


Comments from the review on Reviewable.io

mollyling commented 9 years ago

Reviewed 8 of 8 files at r1. Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


Comments from the review on Reviewable.io

kennysong commented 9 years ago

Review status: 5 of 8 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


src/nat-pmp.js, line 233 [r1] (raw file): Done.


src/pcp.js, line 255 [r1] (raw file): Done.


src/utils.js, line 114 [r1] (raw file): Done.


Comments from the review on Reviewable.io