[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #31558 [Metrics/CollecTor]: Process bridge pool assignments again
#31558: Process bridge pool assignments again
-------------------------------+------------------------------
Reporter: karsten | Owner: metrics-team
Type: enhancement | Status: needs_review
Priority: Medium | Milestone:
Component: Metrics/CollecTor | Version:
Severity: Normal | Resolution:
Keywords: BugSmashFund | Actual Points:
Parent ID: | Points:
Reviewer: irl | Sponsor:
-------------------------------+------------------------------
Changes (by karsten):
* status: needs_revision => needs_review
Comment:
Replying to [comment:9 fava]:
> Replying to [comment:7 karsten]:
>
>
> > Please review
[https://gitweb.torproject.org/user/karsten/collector.git/commit/?h=task-31558&id=db2a6bdab1bfc12377c515577589aa67d34fa2ba
commit db2a6bd in my task-31558 branch].
>
> Hi karsten,
>
> it is my first review so let me know if I could do better.
>
> 1. Please order statement following
[https://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141855.html#1852
Java guidelines] . This guidelines was been written in 1999 but is a good
point to start and it is used also in static code analysis
We do have guidelines for ordering parts in a class:
https://trac.torproject.org/projects/tor/wiki/org/teams/MetricsTeam/MetricsJavaStyleGuide
What we'd really need is a way to automatically check this, just like how
we have Checkstyle to check code style violations. Would you want to try
out some tools that can do these checks? Requirement is that we can obtain
them using `ant resolve` (using Ant Ivy internally) and that we can run
them from Ant just like we're running Checkstyle. (This would be something
for a new ticket.)
> 2. Probably it make sense to create a constant for string
"BridgePoolAssignments"
One might think so, but these two strings actually mean something
different. I'd rather not want to use a single constant for them, because
that would imply that they're the same thing. What I'd really want is give
up on using these string constants, but that's a larger refactoring than
I'd want to do at the moment.
> 3. Instead of evaluate each time `330L * 60L * 1000L` it is better to
create a constant with meaningfull name
This is already changed in the more recent branch.
> 4. Instead of evaluate each time `3L * 24L * 60L * 60L * 1000L` it is
better to create a constant with meaningfull name
I changed this in
[https://gitweb.torproject.org/user/karsten/collector.git/commit/?h=task-31558&id=9c1acf80a14eec1f54f456f23200a5ff70cd9c0f
commit 9c1acf8 in my task-31558 branch].
> 5. startProcessing it seems to complex, is it possible to split in more
separated and testeable methods?
I already changed this in the more recent branch, too.
> 6. there are some for each loop, is it make sense use a functional
approach using lambda?
Maybe, but to me this falls into the category of prettying code rather
than reviewing that it's good to go in. It's more like the try-with-
resource stuff that we could do anytime but which shouldn't block the
merge in this case.
> Please let me know if I could help you in some way,
>
> Best Regards
Thanks for doing this review!
Here's one suggestion for the next review: Be sure to not only review the
first commit, but also consider any follow-up commits in that branch. The
way I'd do that is check out the branch and look at `git diff
$beforefirstcommit..`, e.g., `git diff 8010084..` in this case.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/31558#comment:11>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs