go icon indicating copy to clipboard operation
go copied to clipboard

Code examples: "Output" for non print statements can be confusing

Open shapito27 opened this issue 2 years ago • 16 comments

Hi, I started to use Exercism for practice in Golang recently and I like you product. Thank you for you job 👍🏼 I have one remark.

I've noticed that https://exercism.org/tracks/go/exercises/annalyns-infiltration has nice code example with Output after fmt.Println:

var archerIsAwake = true
var prisonerIsAwake = false
var petDogIsPresent = false
fmt.Println(CanFreePrisoner(knightIsAwake, archerIsAwake, prisonerIsAwake, petDogIsPresent))
// Output: false

but if you take a look at this example https://exercism.org/tracks/go/exercises/vehicle-purchase (and most of other)

needLicense := NeedsLicense("car")
// Output: true

it also has Output but there is no function to Print variable needLicense. It can be confusing. I would recommend to use one style in each task: use fmt.Println() before // Output: or show value of variable in other way.

shapito27 avatar Apr 06 '22 02:04 shapito27

Good point.

ahmedsameha1 avatar Apr 09 '22 13:04 ahmedsameha1

I agree that showing "Output" when there's not a print instruction can be confusing, as there is no output. It's probably a good idea to remove those instances.

Other ways to express the idea can be to put the value in the same line of the variable or explicitly saying what the variable is == to:

needLicense := NeedsLicense("car")   // true

or:

needLicense := NeedsLicense("car")
// true

or even:

needLicense := NeedsLicense("car")
// needLicense == true

I tend to prefer the first two forms as they are compact and descriptive, while not having the confusing "Output" bit.

Since this is an issue that can affect several exercises, my suggestion would be to use this issue as a reference to all PRs that fix this. If someone wants to fix a particular exercise where they see this, it's ok to create a PR for a specific exercise, just make sure to reference this issue for easy tracking.

andrerfcsantos avatar Apr 09 '22 14:04 andrerfcsantos

I believe the good first issue label would be a good fit for this one.

TristanAppDev avatar Apr 11 '22 19:04 TristanAppDev

I always thought the general logic of these type of comments is that they should be written in a REPL fashion. Coming from that perspective, here some comments from my side:

  • I think removing Output when there is no print statement is good.
  • I don't think we should have print statements everywhere because that clutters the examples a lot.
  • I don't think the first and second examples are good because in the REPL logic (which a lot of people are used to from other languages), those examples would mean the return value of the assignment is true which is not the case in Go.
  • The assignment in the first and second example does not add value imo.

I would recommend to use this format as it is clear enough imo and does not include any excess code that was only added for the purpose of writing the example:

NeedsLicense("car")
// true

(Same as at the bottom of vehicle purchase, just without the "Output".)

Just for completion, the correct format with print would be:

needLicense := NeedsLicense("car")
fmt.Println(needLicense)
// Output: true

@andrerfcsantos @norbs57 WDYT about my suggestion above? In case we can agree on the two desired versions, we should add those somewhere in the track documentation.

junedev avatar Apr 12 '22 20:04 junedev

@junedev That makes sense. I like it as a standard going forward.

andrerfcsantos avatar Apr 12 '22 20:04 andrerfcsantos

I agree with most of the points raised.

With regards to the "results of assignments", in the recent parsing_log_files example, I used the following notation taken from the C# track:

b = re.MatchString("[a12]")       // => true
b = re.MatchString("12abc34(ef)") // => true
b = re.MatchString(" abc!")       // => true
b = re.MatchString("123 456")     // => false    

This notation is pretty concise, but I feel ambivalent about those arrows. On the one hand, it is neat to use an arrow to indicate something being "returned". On the other hand, the arrow goes in the wrong direction from an "assignment semantics" point of view, e.g. it should be b ← true.

What do others think about this style?

norbs57 avatar Apr 13 '22 17:04 norbs57

Also, I saw on the C# track that some of the examples were written just as function calls, e.g. the above example would then be written as

re.MatchString("[a12]")       
// => true
re.MatchString("12abc34(ef)") 
// => true
re.MatchString(" abc!")       
// => true
re.MatchString("123 456")     
// => false

Any thoughts?

norbs57 avatar Apr 13 '22 17:04 norbs57

My understanding was that the arrows are used because they appear in some form in the actual REPL that comes with the language. Not sure how it is for C#. Because a REPL is not a standard thing in Go, I am not sure it makes sense to include it.

I wouldn't worry about the direction of the arrow too much because as noted in the second comment by @norbs57, often the assignment can be removed from the example.

Re with or without error, I asked in the Go channel on Slack what people prefer.

junedev avatar Apr 13 '22 18:04 junedev

This probably doesn't help, but in case it does - here are two examples of the Python REPL. The first is the standard one that ships with Python. Note that the arrows are for inputs, and the "output"/"value" or "return" doesn't have them:

2022-04-13_12-08-44

The second 2 examples are from iPython, an "enhanced" REPL that is often used in data sci. Note that here there are explicit in and out lines. However, this is a bit misleading since out is not necessarily output. Sometimes, it's the value of a variable, sometimes the result of calling a function.

2022-04-13_12-03-26

2022-04-13_12-13-23

BethanyG avatar Apr 13 '22 19:04 BethanyG

@BethanyG I know that officially we should use the some standard REPL format for the examples. But tbh even if a standard REPL would exist for Go, I still find this

NeedsLicense("car")
// => true

more readable than this

>>> NeedsLicense("car")
true

Do you actually always use some official REPL format in Python as described here https://exercism.org/docs/building/markdown/markdown#h-code?

junedev avatar Apr 13 '22 20:04 junedev

I realized that there is an issue with the version without the arrow. It gets confusing when there is an additional explaining comment:

NeedsLicense("car")
// true
// This is true because ...

Also people on Slack voted for the arrow so let's go with that.

junedev avatar Apr 13 '22 20:04 junedev

@junedev For the scope of this issue, should the assignee apply the arrowed comment to all exercises?

nahuakang avatar Apr 21 '22 08:04 nahuakang

I would treat the issue as a general one. So in the end it would be great to have consistent examples with either the arrow or print syntax in all concept exercises and concepts. And of course updated documentation of what we decided for here.

junedev avatar Apr 21 '22 10:04 junedev

To give better visibility on exactly what needs to be done and how to contribute to this, I created this new issue with the result of the discussion here.

andrerfcsantos avatar Apr 25 '22 08:04 andrerfcsantos

I am re-opening this because I think the track readme was not yet updated to include the guidelines we settled on here. This is also not mentioned in the new issue linked above so let's use this one here to track the readme update.

junedev avatar Sep 17 '22 21:09 junedev

Here for reference the final formats we used:

Without print:

NeedsLicense("car")
// => true

With print:

needLicense := NeedsLicense("car")
fmt.Println(needLicense)
// Output: true

junedev avatar Oct 01 '22 16:10 junedev