search

Team-4536 / Command-Based-2015

0 stars 0 forks source link

confusing speedCurve method #2

Closed inkling16 closed 9 years ago

inkling16 commented 9 years ago

Utilities speedCurve(double input, double curve) method: return statements look too much alike, create new variable above so that the same methods are not used twice.

Remember, goal is not to minimize lines, but to maximize readability.

stepansubbotin commented 9 years ago

Liam wrote that code. There are a couple of things that I don't really understand about it. Why are you using the absolute value of the curve when we are just limiting it after? Why not just limit it between 0.1 and Double.MAX_VALUE? Also, I don't really understand how the implementation for negative inputs is going to work. You're just doing Math.pow(input, curve) after the curve has been limited. Curves that are fractional have undefined outputs for negative numbers. Equations like y=x^2.4 or y=x^1.43 don't have outputs for negative numbers.

inkling16 commented 9 years ago

He is confusing himself and all of us by trying to cram as much as he can into one line.

Maybe he meant to take the absolute value of "input," not curve? I agree that this code will not work as written.

inkling16 commented 9 years ago

Also, in the "@param curve" comment, the range of valid values for curve should be specified.

stepansubbotin commented 9 years ago

I edited my previous comment. I meant between 0.1 and Double.MAX_VALUE, not between -1 and Double.MAX_VALUE. I also meant that curves that are fractional do have undefined outputs for negative numbers, not don't.

stepansubbotin commented 9 years ago

I just pushed my own implementation of speedCurve to a new branch called "StepanSpeedCurve"

https://github.com/Team-4536/Command-Based-2015/blob/StepanSpeedCurve/src/org/usfirst/frc/team4536/robot/Utilities.java

There are a lot of comments, but I think I'm mostly explaining why I'm doing things, and not what I'm doing. If you think my code makes sense, I can merge it into the master branch.

inkling16 commented 9 years ago

Looks good, just lose some of the unnecessary comments.

stepansubbotin commented 9 years ago

Done and merged

AnonymousAnion commented 9 years ago

[?][?] you guys have been so mean while I've been gone!

Code looks good though. [?]

On Mon, Jun 29, 2015 at 6:12 PM, Stepan Subbotin notifications@github.com wrote:

Done and merged

— Reply to this email directly or view it on GitHub https://github.com/Team-4536/Command-Based-2015/issues/2#issuecomment-116876440 .

inkling16 commented 9 years ago

I just realized that the name "Issues" can have a very negative connotation, so I'm going to close this issue with a cool picture.

image Liam, we're just helping you to further your future career as an astronaut. Just remember, if you make a mistake like that when you're up there, you gonna be dead.

AnonymousAnion commented 9 years ago

lol

On Mon, Jun 29, 2015 at 9:29 PM, Caleb Sykes notifications@github.com wrote:

Closed #2 https://github.com/Team-4536/Command-Based-2015/issues/2.

— Reply to this email directly or view it on GitHub https://github.com/Team-4536/Command-Based-2015/issues/2#event-343683002 .