Open cmarley3-14 opened 4 years ago
As is expected, with 10 weeks to be able to code, there's not a lot of error or improvements in logic. In fact, the sequencing is quite logical. A pro of using Command Based programming. However, the pitfall of Command Based is having too many files. Going through every tree, this year's code has 85 files inside robot2020. I think that's rather ridiculous, but that's coming from a Timed Robot guy with only 10 files. And then I decided to head over and check out 254's 2019 code, and holy cow! I'm not going to bother counting how many files there are!
Anyways, I didn't have much to work with based on what you submitted in the CD Google Form. Nothing special I should notice, nothing I should look out for. I did what I could. If there are any questions, reply to the issue and I'll get back within 3 days.
In General:
L85
, nice inline if statement, though my friends might disagree.RobotContainer
really is the heart of the program, but I have a few nitpicky issues here and there in the little files. These are small, unimportant things that do not require focus. There's so many files I couldn't take too much time to peruse them so I glanced at about 25. Here's a basic compilation of what I noticed and where I found them:
LocalizationManager.java
- utils.Timer
is immediately followed by utils.*
. Kind of defeats the purpose. Of course, nothing huge, but did you expect anything glaring? If the robot works, wonderful! If you had an issue, it could've been put on the CD Google Form. Or you can add it here and I can look around! :wink:RamseteConfig.java
- I look at kaVoltSecondsSquaredPerMeter
and I think, "that could be shorter." Like ka_Vs2_m
. In other files, that wouldn't require me to scroll to the side constantly to read lines of code. It's not a pain in VSCode, but on GitHub, I might as well chew nails.IndexerBallQueueSequence.java
, and then you have IndexerBallQueueSequenceOld.java
Encoder.java, L10
- Just my compliments on the ternary operator. Another thing my friends aren't fond of.MiniPID.java
- Doubles are automatically initialized to zero. That can condense L12, L242 for example. Also, I might consider reverting back to inline if statements (like up in RobotContainer L85
) to keep things small.ModifiedMiniPID.java
- Seems like this could be combined with the above in one way or another, just to help clean up clutter. After all, I hardly see C
used in the code. And it seems to be only externally modified. Correct me if I'm wrong here.Timer.java L138
- Two methods for the same thing? (You begin to notice that these comments point out unimportant details, right? Think of it this way, the code doesn't have huge problems that I notice. That's the bright side. Then again, I'm a command-based noob.)IntakeRun.java execute()
- That's an interesting series of Timers. Something could be improved on in the Timer class I think. Also, at L29, * 1 / 2
is rather confusing.Climber.java L135
- Allow me to borrow this for my team. It's a brilliant idea. We haven't used enums for 2 years!Indexer.java L32
- So, how can you afford that many significant digits?IntakePercent.java
- I'm sorry, is this file used? I couldn't find it anywhere.That cleans up all I have to say. If I have free time the rest of spring, I might keep looking over the code, but I doubt it. If you've had enough of me, close the issue and I won't bother your team anymore. I hope this helped you even marginally, and good luck from here on out!
Thanks for the feedback! With all the craziness of quarantine I missed this, so apologies for the late reply. We really appreciate the feedback and will definitely take it into consideration for the upcoming season.
Best of luck to you and your team!
Hello! I am Christopher Marley, a co-lead programmer of FRC Team 4681, Murphy's Law, and I have been assigned your repository as part of the Chief Delphi "2020 Code Review Party". Over the next two weeks, I will be plumbing the depths of your code and compiling everything in this one issue (so your emails aren't 'pinged' too much). Quick disclaimer, our team uses
TimedRobot
, so forgive me if my knowledge of command based is lacking. I did do a lot of research on the features, so I should be up to speed. If you have any questions at all, reply to this issue or directly email me at chrismarley3.14@gmail.com. I'm very surprised there have been no issues opened in this repo... cool!