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

Concept exercise: Template string and Ternary #1428

Merged
merged 27 commits into from
Oct 23, 2021

Conversation

pertrai1
Copy link
Contributor

@pertrai1 pertrai1 commented Oct 12, 2021

closes #1009

@pertrai1 pertrai1 marked this pull request as draft October 12, 2021 12:26
@junedev
Copy link
Member

junedev commented Oct 13, 2021

@pertrai1 Regarding the root level config.json for this once you get there: Instead of creating a new "concepts" entry for template-strings, please rename the strings-formatting concept instead, keep the same UUID though. We decided to include the string formatting content (mainly that there is no string formatting) in template strings. Also please delete the existing dummy string-formatting concept folder.

@pertrai1
Copy link
Contributor Author

@junedev and @SleeplessByte, can you look at my last 2 commits to see if I am on the right track for setting up exercise concept and the changes I made for config? I am going to start the port of custom signs that is part of swift track next

@junedev
Copy link
Member

junedev commented Oct 14, 2021

Looks good so far! You can set the exercise status directly to "beta", otherwise if we don't change it and merge, the exercise will not show up on the website.

@pertrai1 pertrai1 marked this pull request as ready for review October 16, 2021 01:19
@pertrai1
Copy link
Contributor Author

@junedev and @SleeplessByte, can you start the review of what has been put together? Look forward to the feedback. Thank you.

@pertrai1 pertrai1 force-pushed the ternary-template-string-concepts branch from f8f4376 to 3387c31 Compare October 16, 2021 11:06
@junedev
Copy link
Member

junedev commented Oct 17, 2021

@pertrai1 This was "un-drafted" but afterwards more commits came in. I am not sure whether you are done/ waiting for review or not. Could you clarify?

@pertrai1
Copy link
Contributor Author

@junedev this is ready for review when you have time. Thank you

@exercism exercism deleted a comment from github-actions bot Oct 19, 2021
Copy link
Member

@junedev junedev left a comment

Choose a reason for hiding this comment

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

The general structure is already quite good but some content and the exercise need some fixes, e.g. it seems there was some copy&paste issues/misses regarding to the template string concept.

As usual, if you address some comments for one of the concept files, make sure to apply the fix in all the places.

I did not review the exercise part in detail yet as I suggested some changes to the instructions that will affect the other files. Makes more sense that I have a closer look at the exercise files after those changes were addressed.

@@ -0,0 +1,5 @@
{
"blurb": "The conditional ternary operator is a condensed form of managing actions based on decisions.",
Copy link
Member

Choose a reason for hiding this comment

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

I had trouble understanding the "actions" and "decisions" part. Here a possible alternative:

Suggested change
"blurb": "The conditional ternary operator is a condensed form of managing actions based on decisions.",
"blurb": "The conditional ternary operator is used to write a condensed expression that returns one of two alternative values based on some condition.",

Comment on lines 3 to 4
The [conditional ternary operator][conditional-ternary-operator] is for working with conditions based on the condition being truthy or falsy.
The operator can be used in place of `if/else if/else`.
Copy link
Member

Choose a reason for hiding this comment

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

  • I would repeat the blurb here as introduction sentence.
  • Leave general links like in line 3 for the "Read More" (links.json) section that is rendered below the text on the website.
  • "else if" is not covered by the ternary operator (we don't want to teach/encourage nested ternarys)
  • I would recommend to show the abstract syntax first (currently line 13) and then the example.
  • "predicate" on line 13 seems a bit fancy and wasn't use anywhere else in the concepts so far

Here a suggestion that would address those points (line 13 needs to be deleted then and "implicit type conversion" should link to the respective concept):

Suggested change
The [conditional ternary operator][conditional-ternary-operator] is for working with conditions based on the condition being truthy or falsy.
The operator can be used in place of `if/else if/else`.
The conditional ternary operator is used to write a condensed expression that returns one of two alternative values based on some condition.
It is often just called "ternary operator".
The name stems from the fact that the operator has three operands: `condition ? consequent-expression : alternative-expression`.
It can be used as replacement for short if-else statements.
Just like for if statements, JavaScript will perform implicit type conversion to evaluate the condition.
If the condition is truthy, the operand on the left-hand side of the colon will be returned.
Otherwise the result of the ternary expression is the operand on the right-hand side of the colon.

Comment on lines 9 to 10
year > 2000 ? true : false;
// => true
Copy link
Member

Choose a reason for hiding this comment

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

  • The output that is shown here is wrong.
  • The example is a bit strange because year > 2000 is already a boolean.

Maybe modify the example to return strings like "in the past years" : "a long time ago". Using the ternary operator for something like this is often done in frontend development.

{
"blurb": "The conditional ternary operator is a condensed form of managing actions based on decisions.",
"authors": ["pertrai1"],
"contributors": ["pertrai1"]
Copy link
Member

Choose a reason for hiding this comment

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

no need to put yourself as contributor if you are already the author, the contributor slot is for people that might modify parts of the concept later

(same for the other config file below)

concepts/template-strings/about.md Show resolved Hide resolved

```javascript
costOf('Congratulations Rob Class of 2021', 'dollars');
// => "Your sign cost 90.00 dollars"
Copy link
Member

Choose a reason for hiding this comment

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

Typo and period missing.

Suggested change
// => "Your sign cost 90.00 dollars"
// => "Your sign costs 90.00 dollars."

concepts/template-strings/introduction.md Show resolved Hide resolved
concepts/template-strings/introduction.md Show resolved Hide resolved

Define the following constant characters which will be used to create signs:

- `EXCLAMATION`: This holds the value "!"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think task 1 and 2 add value in teaching the two concepts we want to teach. It was different in the swift exercise where they teach "characters" for example which don't exist in JS. Forcing a ternary operator in task 3 via those constants seems unnecessary strange.

I would recommend to fix this the following way:

  • use buildSign as first task but just going for a simple exemplar like this Happy ${occasion} ${name}!, this will teach to embed strings
  • create a new second task that makes use of the ternary operator, e.g. buildBirthdaySign that accepts an age and does something like this Happy Birthday! What a ${age > 50 ? "young" : "old"} fellow you are...
  • graduationFor is the third task that teaches new line and embedding numbers
  • costOf is the fourth task that teaches toFixed and embedding a calculation


describe('buildSign', () => {
test('built for a birthday', () => {
expect(buildSign(BIRTHDAY, 'Rob')).toBe('Happy Birthday Rob!');
Copy link
Member

@junedev junedev Oct 19, 2021

Choose a reason for hiding this comment

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

make sure all test cases in this file are different from the example in the instructions, once you don't have to deal with the constants for the occasions anymore this also becomes easier here

@pertrai1
Copy link
Contributor Author

Thanks for the detailed feedback @junedev. I will get these updates in the next day.

Copy link
Member

@SleeplessByte SleeplessByte left a comment

Choose a reason for hiding this comment

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

Apparently my review never submitted. There will probably be double items because @junedev also did a review. I apologise for that. Ignore those!

@@ -0,0 +1,5 @@
{
"blurb": "The conditional ternary operator is a condensed form of managing actions based on decisions.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"blurb": "The conditional ternary operator is a condensed form of managing actions based on decisions.",
"blurb": "The conditional ternary operator is a condensed form of executing expressions based on a condition.",

Maybe better to use the terms we also use in other documents?

{
"blurb": "The conditional ternary operator is a condensed form of managing actions based on decisions.",
"authors": ["pertrai1"],
"contributors": ["pertrai1"]
Copy link
Member

Choose a reason for hiding this comment

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

Not that I care, but I think you don't need to be listed in both 😄

@@ -0,0 +1,15 @@
# About

The [conditional ternary operator][conditional-ternary-operator] is for working with conditions based on the condition being truthy or falsy.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The [conditional ternary operator][conditional-ternary-operator] is for working with conditions based on the condition being truthy or falsy.
The [conditional ternary operator][conditional-ternary-operator] is for working with expressions based on the condition being truthy or falsy.

# About

The [conditional ternary operator][conditional-ternary-operator] is for working with conditions based on the condition being truthy or falsy.
The operator can be used in place of `if/else if/else`.
Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to be here twice?

Suggested change
The operator can be used in place of `if/else if/else`.
The operator can be used in place of `if/else`.

Copy link
Member

Choose a reason for hiding this comment

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

I later realised you meant to convey:

if { }
else if { } 
else { }

but that's not the case. Ternary maps to:

if { }
else { }

// => true
```

There are 3 parts that make up the operator: `predicate ? consequent-expression : alternative-expression`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
There are 3 parts that make up the operator: `predicate ? consequent-expression : alternative-expression`
There are 3 parts that make up the operator: `predicate ? consequent-expression : alternative-expression`

Nice.


## 1. Create a set of useful strings

- [const][mdn-const] are blocked scoped that have a value that can't be changed through reassignment.
Copy link
Member

Choose a reason for hiding this comment

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

"const are blocked scoped" doesn't seem right. Perhaps we can rephrase this?


## 2. Create a set of useful characters

- same as #1
Copy link
Member

Choose a reason for hiding this comment

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

Please repeat the hint instead.


## 3. Combine phrases to build up messages

- [template strings][mdn-template-strings] - Template literals - allow for substitution of strings and embedded expressions
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- [template strings][mdn-template-strings] - Template literals - allow for substitution of strings and embedded expressions
- [template strings][mdn-template-strings] (Template literals) allow for substitution of strings and embedded expressions

## 3. Combine phrases to build up messages

- [template strings][mdn-template-strings] - Template literals - allow for substitution of strings and embedded expressions
- [ternary operator][mdn-ternary-operator] is a short-hand way of operating on conditions, similar to `if/elseif/else`. It can be easier to use in template strings because it is condensed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- [ternary operator][mdn-ternary-operator] is a short-hand way of operating on conditions, similar to `if/elseif/else`. It can be easier to use in template strings because it is condensed.
- [ternary operator][mdn-ternary-operator] is a short-hand way of operating on conditions, similar to `if/else`. It can be easier to use in template strings because it is condensed.


Define the following constant strings which will be used to create signs:

- `BIRTHDAY`: This holds the value "Birthday"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `BIRTHDAY`: This holds the value "Birthday"
- `BIRTHDAY`: This holds the string "Birthday"

@pertrai1
Copy link
Contributor Author

Should we add tagged template and remove it from being out of scope?

@junedev
Copy link
Member

junedev commented Oct 20, 2021

Should we add tagged template and remove it from being out of scope?

@pertrai1 I think the current scope is good as is, no need to delay this PR to add more.

@SleeplessByte Tagged templates are consciously mentioned in "out of scope".

@SleeplessByte
Copy link
Member

Perfect. Then let's goooo

@pertrai1
Copy link
Contributor Author

Sorry found a couple spelling mistakes to commit after requesting latest code review.

Copy link
Member

@junedev junedev left a comment

Choose a reason for hiding this comment

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

Thanks for updating everything, especially the exercise. I reviewed the exercise as well now. A couple more things that need to be addressed, then this should be good to go.

lines`;
```

To implement logic into template string syntax:
Copy link
Member

Choose a reason for hiding this comment

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

This exercise introduction is missing a small part on the ternary operator. Maybe above the example below. You already had a version like that before, not sure what happened to it.


## 5. Compute the cost of a sign

Implement the function `costOf(sign, currency)` which takes a string that holds the contents of the sign and a string that represents the currency and returns a phrase that includes the cost to create the sign, formatted with a fixed point notation set to 2 points, followed by the currency string.
Copy link
Member

Choose a reason for hiding this comment

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

  • The information how much one letter costs and the base price for the sign seem to be missing. Either it needs to be a parameter of the function or mentioned in the instructions.
  • The sentence on line 37 is very long. Maybe split it into two sentences.

@@ -0,0 +1,11 @@
{
"blurb": "Learn about characters by helping a sign company create custom messages for their signs.",
Copy link
Member

Choose a reason for hiding this comment

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

This blurb should say "Learn about template strings and the ternary operator ..."

* @param {string} sign
* @param {string} currency
*
* @returns {number} cost to create the sign
Copy link
Member

Choose a reason for hiding this comment

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

The result is a string.

}

/**
* Build a string formatted on multiple lines.
Copy link
Member

Choose a reason for hiding this comment

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

The comment should fit the to the function, e.g. in this case:
"Build a graduation sign that includes multiple lines." might be a better fit here.
(Same applies some other comments in this file.)


describe('buildSign', () => {
test('occasion is Birthday', () => {
expect(buildSign('Birthday', 'Rob')).toBe('Happy Birthday Rob!');
Copy link
Member

Choose a reason for hiding this comment

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

Make sure there are at least 2 test cases per function otherwise everything can just be hard coded to pass the test. While we don't optimize the tests against cheating in general, having more than one test case to ensure there was actually some code there is in scope.

Also remember that the test cases should be different to the examples in the instructions.

In JavaScript, _template strings_ allows for embedding expressions in strings, also referred to as string interpolation.
This functionality extends the functionality of the built in [`String`][string-reference] global object.

You can create template strings in javascript by wrapping text in backticks.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You can create template strings in javascript by wrapping text in backticks.
You can create template strings in JavaScript by wrapping text in backticks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always seem to have a problem spelling that right - almost every time :-)

In JavaScript, _template strings_ allows for embedding expressions in strings, also referred to as string interpolation.
This functionality extends the functionality of the built in [`String`][string-reference] global object.

You can create template strings in javascript by wrapping text in backticks.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You can create template strings in javascript by wrapping text in backticks.
You can create template strings in JavaScript by wrapping text in backticks.

In JavaScript, _template strings_ allows for embedding expressions in strings, also referred to as string interpolation.
This functionality extends the functionality of the built in [`String`][string-reference] global object.

You can create template strings in javascript by wrapping text in backticks.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You can create template strings in javascript by wrapping text in backticks.
You can create template strings in JavaScript by wrapping text in backticks.

}

export function costOf(sign, currency = 'dollars') {
const convertedSign = Number.parseFloat(sign.length);
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this line is necessary. sign.length is already a number.
(If this is removed, the hint also needs to change.)

@pertrai1 pertrai1 requested a review from junedev October 22, 2021 00:06
@SleeplessByte
Copy link
Member

/sync

@pertrai1 pertrai1 force-pushed the ternary-template-string-concepts branch 2 times, most recently from f26e11e to eb88316 Compare October 22, 2021 11:29
@junedev junedev added x:size/large Large amount of work hacktoberfest-accepted Opt-in to hacktoberfest labels Oct 23, 2021
@pertrai1
Copy link
Contributor Author

pertrai1 commented Oct 23, 2021

Thank you so much for the help @junedev. I am hoping after having this first one complete that I will get better at these

@junedev junedev merged commit 21cc2cd into exercism:main Oct 23, 2021
@junedev
Copy link
Member

junedev commented Oct 23, 2021

@pertrai1 I added some more test cases and did some minor wording changes. Hope that is ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Opt-in to hacktoberfest x:size/large Large amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement new Concept Exercise: Template Strings and Ternary
3 participants