Skip to content

London | Samira Hekmati | Module Tools | Week 2 | jq #42

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

samirahekmati
Copy link

@samirahekmati samirahekmati commented Mar 11, 2025

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

@samirahekmati samirahekmati added 📅 Sprint 2 Assigned during Sprint 2 of this module Needs Review Participant to add when requesting review labels Mar 11, 2025
Copy link
Member

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Looking good! This all works well, but I left a few questions about some specifics :)

@@ -5,3 +5,5 @@ set -euo pipefail
# The input for this script is the scores.json file.
# TODO: Write a command to output the names of each player, as well as their city.
# Your output should contain 6 lines, each with two words on it.

jq -r '.[] | "\(.name) \(.city)"' scores.json
Copy link
Member

Choose a reason for hiding this comment

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

In script-03 you used join and here you used string interpolation - both work, and both could be used in both places - do you have any thoughts on which you prefer / which is more clear?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you so much Daniel, @illicitonion for your feedback! I agree that both methods join(", ") and string interpolation are valid, and it's interesting to think about their usage in different contexts.

For example in script-03, I chose join(", ") as I felt it was a cleaner way to handle the array of values (in this case, name and profession). It also keeps the structure clear by dealing with the array directly. On the other hand, in script-05, I used string interpolation because it was more intuitive for handling the dynamic insertion of values within a string.

I don't have a strong preference for one over the other, but I can see how string interpolation may be more flexible, especially if we were working with more complex strings or needing more control over formatting.

I think, if clarity and simplicity are the primary goals, join(", ") works well for array type of structures, while string interpolation might be more readable when we're dealing with individual elements or values.

jq/script-07.sh Outdated
@@ -6,3 +6,5 @@ set -euo pipefail
# TODO: Write a command to output just the names of each player along with the score from their last attempt.
# Your output should contain 6 lines, each with one word and one number on it.
# The first line should be "Ahmed 4" with no quotes.

jq -r '.[] | .scores[-1] as $scores | "\(.name) \($scores)" ' scores.json
Copy link
Member

Choose a reason for hiding this comment

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

$scores seems like a bit of a misleading variable name here, can you think why? What could be more clear?

Copy link
Author

@samirahekmati samirahekmati Apr 1, 2025

Choose a reason for hiding this comment

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

I get what you're trying to say. The variable scores can be misleading because it implies it holds all the scores, but it actually holds just the last score. Renaming it to something like lastScore would make it clearer that it's referring to the most recent score, not the entire list.

jq/script-09.sh Outdated
@@ -6,3 +6,5 @@ set -euo pipefail
# TODO: Write a command to output just the names of each player along with the total scores from all of their games added together.
# Your output should contain 6 lines, each with one word and one number on it.
# The first line should be "Ahmed 15" with no quotes.

jq -r '.[] | "\(.name) \([.scores[]] | add)"' scores.json
Copy link
Member

Choose a reason for hiding this comment

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

What does [.scores[]] here do? Could you write this more simply?

Copy link
Author

Choose a reason for hiding this comment

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

The part [.scores[]] is iterating over each score for every player, then the add function is summing those scores together.
.scores[] is a shorthand way to work with each element inside the scores array. It pulls out each score and feeds it into the add function, which then sums them up. It works, but it's a bit more complicated than needed.
To simplify it, I can directly sum the scores by using the add function on the .scores array without needing the extra [].

jq -r '.[] | "\(.name) \((.scores | add))"' scores.json

@illicitonion illicitonion added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Mar 18, 2025
@illicitonion illicitonion added Complete Volunteer to add when work is complete and review comments have been addressed and removed Reviewed Volunteer to add when completing a review labels Apr 1, 2025
@illicitonion
Copy link
Member

Perfect - thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Volunteer to add when work is complete and review comments have been addressed 📅 Sprint 2 Assigned during Sprint 2 of this module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants