Chief Delphi 2020 Code Review Party

[cmarley3-14]

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 [email protected].
I'm very surprised there have been no issues opened in this repo... cool!

[cmarley3-14]

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:

• So far, I'm really liking how well documented and commented the program is. I prefer how subsystems and commands are traditionally separated, but I see why one might prefer this method, and I don't get confused either. So long as it works.

RobotContainer.java:

• First off, I learned something new about nested classes. I prefer to keep things simple in Java, but it's always nice seeing how things are in the real world.
• It's my preference to put the constructor near the top, and then have the methods that are called in the constructor defined below.
• L85, nice inline if statement, though my friends might disagree.
• The code for autonomous is all over the place in RobotContainer: the enum, two or three functions, commands, etc., but maybe that's my fault for not having worked with command-based before. And then the point of the enum gets scrapped, which confused me for a second because there's no explanation as to why the autonomous logic is commented out.
• The emergency disable is a bit ridiculous. What happens if one of the buttons breaks and you don't know? My team has had broken button problems which is why I bring it up.

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! 😉
• 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.
• You have 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!

[natesales]

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!