Conversation
Summary of ChangesHello @Roxie222, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on optimizing memory usage and improving performance within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great optimization that moves list instantiations into static final fields to improve performance and reduce memory allocations. The refactoring of getNearestBranch also simplifies the code. My feedback focuses on improving code style and maintainability by adhering to standard Java naming conventions for constants and method parameters, which will make the code more readable and consistent.
| private static final List<Pose2d> BlueBranches = | ||
| List.of( | ||
| blueBranchA, | ||
| blueBranchB, | ||
| blueBranchC, | ||
| blueBranchD, | ||
| blueBranchE, | ||
| blueBranchF, | ||
| blueBranchG, | ||
| blueBranchH, | ||
| blueBranchI, | ||
| blueBranchJ, | ||
| blueBranchK, | ||
| blueBranchL); | ||
|
|
||
| private static final List<Pose2d> RedBranches = | ||
| List.of( | ||
| redBranchA, | ||
| redBranchB, | ||
| redBranchC, | ||
| redBranchD, | ||
| redBranchE, | ||
| redBranchF, | ||
| redBranchG, | ||
| redBranchH, | ||
| redBranchI, | ||
| redBranchJ, | ||
| redBranchK, | ||
| redBranchL); | ||
|
|
||
| private static final List<Pose2d> LeftBlueReefFaces = | ||
| List.of( | ||
| lBlueReefFaceAB, | ||
| lBlueReefFaceCD, | ||
| lBlueReefFaceEF, | ||
| lBlueReefFaceGH, | ||
| lBlueReefFaceIJ, | ||
| lBlueReefFaceKL); | ||
|
|
||
| private static final List<Pose2d> LeftRedReefFaces = | ||
| List.of( | ||
| lRedReefFaceAB, | ||
| lRedReefFaceCD, | ||
| lRedReefFaceEF, | ||
| lRedReefFaceGH, | ||
| lRedReefFaceIJ, | ||
| lRedReefFaceKL); | ||
|
|
||
| private static final List<Pose2d> RightBlueReefFaces = | ||
| List.of( | ||
| rBlueReefFaceAB, | ||
| rBlueReefFaceCD, | ||
| rBlueReefFaceEF, | ||
| rBlueReefFaceGH, | ||
| rBlueReefFaceIJ, | ||
| rBlueReefFaceKL); | ||
|
|
||
| private static final List<Pose2d> RightRedReefFaces = | ||
| List.of( | ||
| rRedReefFaceAB, | ||
| rRedReefFaceCD, | ||
| rRedReefFaceEF, | ||
| rRedReefFaceGH, | ||
| rRedReefFaceIJ, | ||
| rRedReefFaceKL); | ||
|
|
||
| private static final List<Pose2d> LeftBlueBranch = | ||
| List.of(blueBranchA, blueBranchC, blueBranchE, blueBranchG, blueBranchI, blueBranchK); | ||
|
|
||
| private static final List<Pose2d> LeftRedBranch = | ||
| List.of(redBranchA, redBranchC, redBranchE, redBranchG, redBranchI, redBranchK); | ||
|
|
||
| private static final List<Pose2d> RightBlueBranch = | ||
| List.of(blueBranchB, blueBranchD, blueBranchF, blueBranchH, blueBranchJ, blueBranchL); | ||
|
|
||
| private static final List<Pose2d> RightRedBranch = | ||
| List.of(redBranchB, redBranchD, redBranchF, redBranchH, redBranchJ, redBranchL); |
There was a problem hiding this comment.
According to standard Java naming conventions, static final fields (constants) should be named in UPPER_SNAKE_CASE. All the newly added lists are constants and should be renamed. For example, BlueBranches should be BLUE_BRANCHES. I also suggest using plural names for lists of branches, like LEFT_BLUE_BRANCHES for LeftBlueBranch, to improve clarity.
There was a problem hiding this comment.
I do recommend using plural names for lists of branches, and would at least use camelCase like the other constants above.
| case LEFTB -> getNearestBranch(pose, LeftBlueBranch, LeftRedBranch); | ||
| case RIGHTB -> getNearestBranch(pose, RightBlueBranch, RightRedBranch); | ||
| case L1LB -> getNearestBranch(pose, LeftBlueReefFaces, LeftRedReefFaces); | ||
| case L1RB -> getNearestBranch(pose, RightBlueReefFaces, RightRedReefFaces); | ||
| case ALLB -> getNearestBranch(pose, BlueBranches, RedBranches); |
There was a problem hiding this comment.
To be consistent with the recommended UPPER_SNAKE_CASE for constants, the usages here should be updated. I've suggested new names that use plural for branch lists (e.g., LEFT_BLUE_BRANCHES) to improve clarity.
| case LEFTB -> getNearestBranch(pose, LeftBlueBranch, LeftRedBranch); | |
| case RIGHTB -> getNearestBranch(pose, RightBlueBranch, RightRedBranch); | |
| case L1LB -> getNearestBranch(pose, LeftBlueReefFaces, LeftRedReefFaces); | |
| case L1RB -> getNearestBranch(pose, RightBlueReefFaces, RightRedReefFaces); | |
| case ALLB -> getNearestBranch(pose, BlueBranches, RedBranches); | |
| case LEFTB -> getNearestBranch(pose, LEFT_BLUE_BRANCHES, LEFT_RED_BRANCHES); | |
| case RIGHTB -> getNearestBranch(pose, RIGHT_BLUE_BRANCHES, RIGHT_RED_BRANCHES); | |
| case L1LB -> getNearestBranch(pose, LEFT_BLUE_REEF_FACES, LEFT_RED_REEF_FACES); | |
| case L1RB -> getNearestBranch(pose, RIGHT_BLUE_REEF_FACES, RIGHT_RED_REEF_FACES); | |
| case ALLB -> getNearestBranch(pose, BLUE_BRANCHES, RED_BRANCHES); |
There was a problem hiding this comment.
This should be updated to match whatever new names are chosen for the constants above.
| private static Pose2d getNearestBranch(Pose2d p, List<Pose2d> Blue, List<Pose2d> Red) { | ||
| List<Pose2d> branchPose2ds = AllianceUtils.isBlue() ? Blue : Red; | ||
| return p.nearest(branchPose2ds); | ||
| } |
There was a problem hiding this comment.
Parameter names should follow Java's lower camel case convention. Blue and Red should be renamed to something like bluePoses and redPoses respectively to improve readability and adhere to conventions.
| private static Pose2d getNearestBranch(Pose2d p, List<Pose2d> Blue, List<Pose2d> Red) { | |
| List<Pose2d> branchPose2ds = AllianceUtils.isBlue() ? Blue : Red; | |
| return p.nearest(branchPose2ds); | |
| } | |
| private static Pose2d getNearestBranch(Pose2d p, List<Pose2d> bluePoses, List<Pose2d> redPoses) { | |
| List<Pose2d> branchPose2ds = AllianceUtils.isBlue() ? bluePoses : redPoses; | |
| return p.nearest(branchPose2ds); | |
| } |
| private static final List<Pose2d> BlueBranches = | ||
| List.of( | ||
| blueBranchA, | ||
| blueBranchB, | ||
| blueBranchC, | ||
| blueBranchD, | ||
| blueBranchE, | ||
| blueBranchF, | ||
| blueBranchG, | ||
| blueBranchH, | ||
| blueBranchI, | ||
| blueBranchJ, | ||
| blueBranchK, | ||
| blueBranchL); | ||
|
|
||
| private static final List<Pose2d> RedBranches = | ||
| List.of( | ||
| redBranchA, | ||
| redBranchB, | ||
| redBranchC, | ||
| redBranchD, | ||
| redBranchE, | ||
| redBranchF, | ||
| redBranchG, | ||
| redBranchH, | ||
| redBranchI, | ||
| redBranchJ, | ||
| redBranchK, | ||
| redBranchL); | ||
|
|
||
| private static final List<Pose2d> LeftBlueReefFaces = | ||
| List.of( | ||
| lBlueReefFaceAB, | ||
| lBlueReefFaceCD, | ||
| lBlueReefFaceEF, | ||
| lBlueReefFaceGH, | ||
| lBlueReefFaceIJ, | ||
| lBlueReefFaceKL); | ||
|
|
||
| private static final List<Pose2d> LeftRedReefFaces = | ||
| List.of( | ||
| lRedReefFaceAB, | ||
| lRedReefFaceCD, | ||
| lRedReefFaceEF, | ||
| lRedReefFaceGH, | ||
| lRedReefFaceIJ, | ||
| lRedReefFaceKL); | ||
|
|
||
| private static final List<Pose2d> RightBlueReefFaces = | ||
| List.of( | ||
| rBlueReefFaceAB, | ||
| rBlueReefFaceCD, | ||
| rBlueReefFaceEF, | ||
| rBlueReefFaceGH, | ||
| rBlueReefFaceIJ, | ||
| rBlueReefFaceKL); | ||
|
|
||
| private static final List<Pose2d> RightRedReefFaces = | ||
| List.of( | ||
| rRedReefFaceAB, | ||
| rRedReefFaceCD, | ||
| rRedReefFaceEF, | ||
| rRedReefFaceGH, | ||
| rRedReefFaceIJ, | ||
| rRedReefFaceKL); | ||
|
|
||
| private static final List<Pose2d> LeftBlueBranch = | ||
| List.of(blueBranchA, blueBranchC, blueBranchE, blueBranchG, blueBranchI, blueBranchK); | ||
|
|
||
| private static final List<Pose2d> LeftRedBranch = | ||
| List.of(redBranchA, redBranchC, redBranchE, redBranchG, redBranchI, redBranchK); | ||
|
|
||
| private static final List<Pose2d> RightBlueBranch = | ||
| List.of(blueBranchB, blueBranchD, blueBranchF, blueBranchH, blueBranchJ, blueBranchL); | ||
|
|
||
| private static final List<Pose2d> RightRedBranch = | ||
| List.of(redBranchB, redBranchD, redBranchF, redBranchH, redBranchJ, redBranchL); |
There was a problem hiding this comment.
I do recommend using plural names for lists of branches, and would at least use camelCase like the other constants above.
| private static Pose2d getNearestBranch(Pose2d p, List<Pose2d> Blue, List<Pose2d> Red) { | ||
| List<Pose2d> branchPose2ds = AllianceUtils.isBlue() ? Blue : Red; | ||
| return p.nearest(branchPose2ds); | ||
| } |
| case LEFTB -> getNearestBranch(pose, LeftBlueBranch, LeftRedBranch); | ||
| case RIGHTB -> getNearestBranch(pose, RightBlueBranch, RightRedBranch); | ||
| case L1LB -> getNearestBranch(pose, LeftBlueReefFaces, LeftRedReefFaces); | ||
| case L1RB -> getNearestBranch(pose, RightBlueReefFaces, RightRedReefFaces); | ||
| case ALLB -> getNearestBranch(pose, BlueBranches, RedBranches); |
There was a problem hiding this comment.
This should be updated to match whatever new names are chosen for the constants above.
3 participants
Moved Branch Lists out of code into statics to reduce memory allocations and improve performance