search

donnell74 / CSC-450-Scheduler

MIT License
1 stars 1 forks source link

Computers #27

Closed prichey closed 10 years ago

prichey commented 10 years ago

This pull request accomplishes 2 things (well, 3, kinda):

1: Radiobutton default value is set, and value is updated properly (I was getting the same problems related to calling bool on a string).

2: I added an okay_to_add_constraint function which calls the corresponding functions to check if a constraint is a duplicate or conflicting with any previous constraints. This cleans up the code significantly and is much more DRY. Related to this, I also swapped the values of check_constraint_exists() to return True if the constraint does exist, and also swapped the values of constraint_adding_conflict() to return True if the constraint does add a conflict. I updated all instances to make this change work just like it used to, just be aware of this swap if you use either of these functions in the future.

3: Made spacing consistent and removed trailing whitespace in the three files I touched.

The real significant changes are in guiConstraints, lines 350 - 430

dhebrink commented 10 years ago

I haven't pulled this down and run it yet, but by looking through the code, it looks good to me. I agree the okay_to_add_constraint function was a good idea. When I was looking through those functions, I thought it was a bit too repeated also.

@prichey , careful going through too much with the fixing little code standard things (whitespace, var names, etc). I planned on using our team meeting tomorrow to make sure that all of us who will be fixing those things are on the same page before we dive in.

prichey commented 10 years ago

@dhebrink that's why I posted in flowdock about it - I didn't hear back from anybody so I assumed it'd be fine to go ahead

CameronHKIng commented 10 years ago

As long as you keep up with the last few constraints we have to make sure the overlapping detection logic and such works for them as well, this looks good. After this sprint, I think we have all the constraints required.

prichey commented 10 years ago

Yeah, I'm happy to revamp those functions to make them work for all constraints. Rather than doing some sort of guessing (like splitting and comparing the length, which doesn't seem optimal), we might consider passing the constraint type explicitly with the constraint.

Regardless, with the constraints that exist currently, this code should work just fine.