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

Improve code with warning of Visual code analysis #5

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

leguims
Copy link

@leguims leguims commented Jun 25, 2018

can fix issue #4

@daancode
Copy link
Owner

Hi ! Thanks for your PR. I'm not convinced about throwing an exception, maybe better solution is to clamp coords to world size or return empty vector if coords are invalid ?

@leguims
Copy link
Author

leguims commented Jun 27, 2018

Return empty vector is like "no way found". But user does not known that "inputs was bad". Exception is more explicit for me.
Clamp to world size is better, but user does not know that coords were bad as input data. Maybe, result with clamped coords could be wrong.

@daancode
Copy link
Owner

I see your point, but still I'd rather avoid using exceptions. What do you think about assertion and enum to indicate internal generator state ?

enum class State
{
    Ready,
    InvalidArguments,
    InternalError,
    Success
}

auto path = generator.findPath({0, 0}, {20, 20});
if(generator.getState() == AStar::State::Success)
{
    // Your path is valid.
}

@leguims
Copy link
Author

leguims commented Jun 28, 2018

Could you explain to me why do you want to avoid exceptions ?

Your solution works, if your prefer it, that's ok for me. But I still prefer exceptions, because you can forget to check "generator.getState()", but not exceptions. That's the advantage of exceptions, you cannot ignore them or you have to do it explicitly.

@leguims
Copy link
Author

leguims commented Jun 28, 2018

The commit "Improve code with warning of Visual code analysis - e7aaf02" use getState() instead of exception. You can take it. Probably the use of "success", "ready" and other states need more code.

Ignore the commit "Replace getState() error management by exception management - d977c96" (and later), because, this is the return of exception, which is good for me, cause I don't want to check getState() each time I use "find" method. This is what I prefer, but I understand that you avoid exception usage. 😃

@daancode
Copy link
Owner

daancode commented Jul 1, 2018

Thank you :) Using the exception forces the way in which the application must deal with incorrect arguments. I think the better solution is to validate coordinates before passing it to the find method. Validation can be done in any way, and its depends on the programmer preferences.

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.

No documentation and protection on 'Vec2i' initialisation
2 participants