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

Minor changes for readability. #261

Closed
wants to merge 8 commits into from
Closed

Conversation

tonypr
Copy link
Collaborator

@tonypr tonypr commented Nov 4, 2023

Some minor style/readability improvements:

  • Simplifies the expandable_nodes computation. Nodes in the connected component should only be from that color, so we can remove the existing color check.
  • We can simplify is_enemy_road by not adding the extra is not None check as it's not needed. It shouldn't be needed for is_enemy_node either.. but that seemed to fail for some reason I didn't get into.
  • Changes the longest_acyclic_path method to prefer .append in the DFS search for performance as .insert(0) shifts the whole list. Would have to use a queue otherwise but it's not necessary.
  • Also simplifies the longest_acyclic_path method by using helper methods for friendly/enemy roads/nodes.

Tested with:
coverage run --source=catanatron -m pytest tests/

Copy link

netlify bot commented Nov 4, 2023

👷 Deploy request for catanatron-staging pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit b8af070

@tonypr tonypr requested a review from bcollazo November 4, 2023 04:29
@bcollazo
Copy link
Owner

bcollazo commented Dec 1, 2023

Cool! Thanks for the contribution.

I fixed the CI issues in master, so updating this branch with master should fix build! 🤞

@tonypr tonypr closed this Dec 2, 2023
@tonypr tonypr deleted the minor_changes_3 branch December 2, 2023 00:43
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