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

Use positive logic when calculating score #476

Merged
merged 1 commit into from
Aug 27, 2023

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Aug 21, 2023

Issue:
In our score code we use refactored logic that does not explicitly follow the documented rules.

For example, when calculating the players score we use negative logic, e.g. player.action != actions.BRAKE , while in the documentation we define the rules using positive language, e.g. if you want to continue forward. return actions.RIGHT or actions.LEFT if you like to bypass the obstacle.

There are reasons why you might want to follow the document's logic explicitly rather than refactoring it:

  • Clarity and Traceability: If someone else (or even you at a later time) refers back to the original documentation to understand or validate the code's behavior, having the logic implemented as closely as possible to the documented rules makes it easier to follow and validate.

  • Avoid Introduction of Bugs: Sometimes refactoring can introduce unintended side effects, especially when the original rules have been tested and validated extensively.

  • Maintenance and Future Updates: If the documentation is updated in the future, it's much easier to identify what parts of the code need to change if the code follows the structure of the documentation.

  • Communication with Stakeholders: If you're communicating with non-technical stakeholders who are familiar with the documented rules but not with the nuances of code, it helps if your code closely mirrors the documentation. This can make discussions and explanations smoother.

In our case, ROSE is intended as educational code for students learning to code, it makes sense to write the score code to explicitly follow the documented rules.

What are the updates proposed by this PR:

  • Use positive logic to follow the documentation, e.g. replace the != with == and not in with in
  • Use score_pickup explicitly, instead of using score_move_forward
  • When moving backword use ``score_move_backward instead of adding score_move_forward
    and then reducing `score_move_backward` twice.
  • Move action clean up to the end of the method
  • Remove six

Possible bug fixes:

  • Current code assume score_move_backward and score_move_forward are identical, if they will change current code logic will break.
  • Currnet code assumes score_move_forward and score_pickup are identical, if they change current code will break.

Ref:
https://github.com/RedHat-Israel/ROSE/tree/master/examples

@yaacov
Copy link
Member Author

yaacov commented Aug 21, 2023

@nirs @sleviim hi, can you review / refer to a reviewer ?

Copy link
Member

@nirs nirs left a comment

Choose a reason for hiding this comment

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

Using positive logic and avoiding assumptions so we can change the rules easily in the future sounds like a good idea. I'm not sure about
the change to the player loop, and I don't think that avoiding the sorting is needed since our N is tiny.

Can you split out the unrelated python 2 changes and the positive logic changes to different commits?

We can consider changing the player loop later if you want.

rose-admin Outdated Show resolved Hide resolved
rose/client/game.py Outdated Show resolved Hide resolved
rose/server/score.py Show resolved Hide resolved
rose/server/score.py Outdated Show resolved Hide resolved
rose/server/score.py Show resolved Hide resolved
rose/server/score.py Show resolved Hide resolved
rose/server/score.py Outdated Show resolved Hide resolved
rose/server/score.py Show resolved Hide resolved
rose/server/score.py Show resolved Hide resolved
rose/server/score.py Outdated Show resolved Hide resolved
Copy link
Member

@nirs nirs left a comment

Choose a reason for hiding this comment

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

Changes look good but comments can be improved.

rose/server/score.py Outdated Show resolved Hide resolved
rose/server/score.py Outdated Show resolved Hide resolved
rose/server/score.py Outdated Show resolved Hide resolved
rose/server/score.py Show resolved Hide resolved
Signed-off-by: yzamir <kobi.zamir@gmail.com>
Copy link
Member

@nirs nirs left a comment

Choose a reason for hiding this comment

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

Thanks!

@nirs nirs merged commit 91606ad into RedHat-Israel:master Aug 27, 2023
1 check passed
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.

None yet

2 participants