search

hlathery / Virtual-Ecosystem

0 stars 0 forks source link

Code Review Comments (Katie Slobodsky) #16

Open katieslobodsky opened 6 days ago

katieslobodsky commented 6 days ago
  1. In post_biome_counts in eco.py, parameterize the SQL statement instead of using an f string to avoid SQL injection
  2. Consider deleting unneeded code (especially in village.py) that is commented out to improve readability
  3. Since name and quantity for buildings are part of the same table, maybe when returning values for get_village_overview() you can combine the name and count and return a single list of dictionaries with name and count in each building item.
  4. In adjust_storage, you can reduce the number of database calls by combining the logic under one “with db.engine.begin() as connection:” instead of 2. This would reduce runtime.
  5. For kill_villager, instead of just returning “OK” maybe you can return a list of dictionaries of deleted villagers information, such as their name, id, and age. This could confirm that the correct villagers were deleted.
  6. In build_structure, make sure to check if the player has enough resources before updating building quantity. You can do this by comparing the requested resources and available resources.
  7. Consider adding more error handling, for example in adjust_storage, in the case where the resource quantity is less than the requested storage adjustment, this would throw an error
  8. There could be more logging, such as when a villager is created or killed, storage is adjusted, or a building is built. This could help with testing.
  9. Instead of looping through each job, you can group villagers by job and update them in a single operation where possible; this can reduce the number of queries
  10. A possible improvement for get_job_list could be to add sorting or filtering for more organization (e.g. sorting by job_name)
  11. For functions that require updating, add more checks to ensure that the update could be completed to avoid miscalculations and mismatches in data. If not, roll back the transaction and ensure that nothing was updated to avoid concurrency errors.
  12. “Entitys” spelled wrong in eco.py - consider changing to “entities”