Skip to content

Conversation

ceebo
Copy link
Contributor

@ceebo ceebo commented Dec 13, 2014

LifeAPI looks great. I hacked around with it a bit. What do you think of these patches?

Patch 1. IMO these headers do not really belong inside LifeAPI.h. Especially comio.h seems not available on Linux.

Patch 2. I hate dealing with strings in C/C++, IMO they should only be used for input/output.

Patch 3. gcc spews a load of warnings unless char * is declared as const

Patch 4. Direct implementations of Contains and ContainsInverse. I don't know if you will like them.

Patch 5. I think these renamings make it more obvious what the function does without needing to read the documenatation. Especially ContainsInverse I think is a tricky name that I would prefer to get rid of. This is probably the most subjective patch I am submitting.

Patch 6. Add function GetBoundary. Completely untested but could be useful to easily defined the "unwanted" set in a LifeTarget.

(Oh I notice you made some changes in the last couple of hours. Hopefully this still applies cleanly?! Maybe not)

@simsim314
Copy link
Owner

Hey I really like the changes, and I'm glad someone is interested as I am in the project.

One point - I've made simple UnitTest - just to make sure nothing is messed up.

If you add some functionality, can you make simple unit test for it as well. If you change the naming, the usage in UnitTest should obviously also be changed.

@simsim314
Copy link
Owner

OK I've merged the branches. I did similar optimization in the Target search - so I left my code that I know works.

The GetBoundary is not optimal, I'm now returning the code to use min-max optimization. The min and max are now made sure to be two cells from the last living cell (or at 0, N-1). So I'm not sure how performance sensitive is GetBoundary, but consider using min-max properties if you want to use it in search on each iteration.

Everything else looks great - I also added small unit test for GetBoundary, and added prints. Also made sure the samples are working (I needed conio for getch, but getchar works as well).

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