Skip to content

Add methods to initialize and access turtleList, and refactor turtle access methods for improved code consistency and readability.#4370

Merged
walterbender merged 9 commits intosugarlabs:masterfrom
Ashrafmuhmed:master
Feb 11, 2025
Merged

Add methods to initialize and access turtleList, and refactor turtle access methods for improved code consistency and readability.#4370
walterbender merged 9 commits intosugarlabs:masterfrom
Ashrafmuhmed:master

Conversation

@Ashrafmuhmed
Copy link
Contributor

Description

This pull request implements the following methods in the Turtles.TurtlesModel class in js/turtles.js related to issue #4369 :

  • Initialize turtleList: Adds a method to initialize the turtleList property.
  • Direct Access to Required Turtle: Adds a method to directly access a turtle by its index or ID, rather than retrieving the entire turtleList.
  • Return Length of turtleList: Adds a method to return the number of turtles in the turtleList.

Tasks

  • Initialize turtleList
  • Direct Access to Required Turtle
  • Return Length of turtleList

@walterbender
Copy link
Member

The new methods look good, but don't we want to use them?

@Ashrafmuhmed
Copy link
Contributor Author

I believe these new methods should be used because they improve encapsulation and make it easier to interact with the turtleList without directly accessing it. This aligns with the TODO suggestion. Do you have any concerns or suggestions?

I'm happy to listen and make adjustments if needed

@walterbender
Copy link
Member

I agree. But adding the methods is only Step 1. Now we need to use them instead of the .get() methods.

@Ashrafmuhmed
Copy link
Contributor Author

Got it! I'll work on replacing the .get() methods with the new ones and update you once it's done.

@Ashrafmuhmed Ashrafmuhmed changed the title Add methods to initialize and access turtleList in TurtlesModel Add methods to initialize and access turtleList and used them to refactor turtle management in TurtlesModel Feb 9, 2025
@Ashrafmuhmed
Copy link
Contributor Author

@walterbender Let me know your thoughts! I also noticed that elements were being added to turtleList directly, so I introduced a new method to handle this process more cleanly.

js/turtles.js Outdated
* @param {Object} turtle
*/
pushTurtle(turtle) {
if(!this._turtleList.includes(this))
Copy link
Member

Choose a reason for hiding this comment

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

should this be includes(turtle)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right! It should be includes(turtle) to check for the existence of the turtle before pushing it in the next line. Thanks for catching that! I'll fix it.

@walterbender
Copy link
Member

There are many references to turtleList outside of turtles.js. We'll need to check those as well to see where we need to make changes.

@Ashrafmuhmed
Copy link
Contributor Author

Yeah, I know. I'm already working on them. I was just looking for your thoughts!

@Ashrafmuhmed Ashrafmuhmed changed the title Add methods to initialize and access turtleList and used them to refactor turtle management in TurtlesModel Add methods to initialize and access turtleList, and refactor turtle access methods for improved code consistency and readability. Feb 11, 2025
@Ashrafmuhmed
Copy link
Contributor Author

Hi @walterbender , I have added methods to initialize and access turtleList. I also put significant effort into refactoring the turtle access methods to improve code consistency and readability. Please review the changes and let me know if any further improvements are needed. Thanks!

@walterbender walterbender merged commit a0d4ece into sugarlabs:master Feb 11, 2025
4 checks 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.

2 participants