Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Common 213 #38

Open
wants to merge 26 commits into
base: development
Choose a base branch
from
Open

Common 213 #38

wants to merge 26 commits into from

Conversation

minhn02
Copy link
Contributor

@minhn02 minhn02 commented Jan 25, 2019

No description provided.

@minhn02 minhn02 requested a review from JasonTStanley January 25, 2019 02:05

if (islowerBound == null) {
if (isLowerBound == null) {
println("bound not upper/lower in condition")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be an exception?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you threw an exception here then maybe you could make resolutionCondition not be lateinit (or maybe just return it from inside the if statements and get rid of the var all together)

return originCondition == originConditionP && conflictingCondition == conflictingConditionP
}

override fun getResolution(currentCondition: Condition<Double>, subsystem: ColleagueSubsystem<Double>): Condition<Double> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should look into just using the Position class I made. I think it does basically what you are trying to do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The position class doesn't tell me the closest bound and doesn't pass back the closest bound though

return originCondition == originConditionPS && conflictingCondition == conflictingConditionPS
}

override fun getResolution(currentCondition: Condition<Double>, subsystem: ColleagueSubsystem<Double>): Condition<Double> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Find a way to not repeat this code in the various condition classes. You are gonna make changes later and forget to update all the different areas.

@minhn02 minhn02 force-pushed the COMMON-213 branch 3 times, most recently from af4a738 to a80ff12 Compare February 2, 2019 01:07
@minhn02 minhn02 force-pushed the COMMON-213 branch 3 times, most recently from 6e0daa0 to d83e675 Compare February 11, 2019 20:42
@minhn02 minhn02 force-pushed the COMMON-213 branch 4 times, most recently from f2ab703 to c93c8d0 Compare February 19, 2019 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants