-
Notifications
You must be signed in to change notification settings - Fork 19
Add style guide updates based on CS19 notes #46
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
ZacharyEspiritu
wants to merge
3
commits into
brownplt:master
Choose a base branch
from
ZacharyEspiritu:cs19-style
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ | |
@title{Pyret Style Guide} | ||
@author{Frank Goodman and Shriram Krishnamurthi} | ||
|
||
Ahoy matey! Here be the style guide for Pyret. Follow me rules to find | ||
Ahoy matey! Here be the style guide for Pyret. Follow me rules to find | ||
the hidden treasure, or walk the plank! | ||
|
||
@(table-of-contents) | ||
|
@@ -19,28 +19,30 @@ You should indent your code blocks using two spaces (not tabs). | |
|
||
@subsection{Line Length} | ||
|
||
Try to keep the total length of your lines under 80 characters. | ||
The total length of your lines should be kept under 80 characters. The Pyret | ||
editor displays a dotted blue at the 80 character mark when any of your lines | ||
are over this limit. | ||
|
||
For overly long lines, it's actually really hard to figure out where | ||
to put in good line breaks. This is in every language. Look for | ||
something that tries to match the logical structure of your program. | ||
|
||
However, there's usually a better way to solve this problem: create | ||
extra variables to name the intermediate pieces, and use those | ||
names. E.g.: instead of worrying where to put in line breaks in | ||
@codedisp{ | ||
@codedisp{ | ||
fun f(x, y, z): | ||
g(some-very-long-thing(x * x, y + y), other-very-long-thing((x + y + z) / (2 * 3 * 4))) | ||
end | ||
} | ||
} | ||
it might be better to write it as | ||
@codedisp{ | ||
fun f(x, y, z): | ||
sensible-name-1 = some-very-long-thing(x * x, y + y) | ||
sensible-name-2 = other-very-long-thing((x + y + z) / (2 * 3 * 4)) | ||
g(sensible-name-1, sensible-name-2) | ||
end | ||
} | ||
} | ||
(and think about indenting/shortening those two new lines). | ||
|
||
Not only does this shorten lines, it makes it clearer what all these | ||
|
@@ -168,7 +170,7 @@ end | |
Even though Pyret does not currently check parametric annotations, you | ||
should still write them for their value as user documentation. Thus: | ||
@codedisp{ | ||
fun length(lst :: List<Any>) -> Number: | ||
fun sum-str-lengths(lst :: List<String>) -> Number: | ||
# ... | ||
end | ||
} | ||
|
@@ -188,6 +190,35 @@ fun sqrt(n :: Number%(non-negative)) -> Number: | |
end | ||
} | ||
|
||
@subsection{Parametric Annotations} | ||
|
||
You should avoid using @tt{Any} as the subtype of a parametric data structure, | ||
like a @tt{List}. | ||
|
||
In general, a @tt{List<Any>} definition is often undesirable because it | ||
indicates elements of any type can be members of the list. In particular, | ||
this subtly implies that multiple elements of different types could exist. | ||
In other words, @tt{[list: 1, "apple", true]} would satisfy the annotation | ||
@tt{List<Any>}. Creating @tt{List}s like this is usually bad practice. | ||
|
||
In Pyret, we can do this by parametrizing the type. Rather than | ||
|
||
@codedisp{ | ||
fun search(lst :: List<Any>) -> Any: ... end | ||
} | ||
|
||
we'd now do something like | ||
|
||
@codedisp{ | ||
fun search<A>(lst :: List<A>) -> A: ... end | ||
} | ||
|
||
Now, the elements of @tt{lst} can be of any type, but all elements of lst will | ||
be of type @tt{A} (along with the return value of @tt{search})! Note that the | ||
actual name of the type parameter is arbitrary (we could have written | ||
@tt{search<SomeOtherName>} instead of @tt{search<A>}), but it is convention to | ||
use a single letter for parametric annotations. | ||
|
||
@subsection{Testing} | ||
|
||
You should test every function you write for both general cases and edge cases. | ||
|
@@ -266,7 +297,7 @@ cases (Animal) a: | |
| dillo(_, _) => ... | ||
end | ||
} | ||
and thus ignore multiple fields. | ||
and thus ignore multiple fields. | ||
|
||
Finally, if your conditional is not designed to handle a particular | ||
kind of datum, signal an error: | ||
|
@@ -277,6 +308,84 @@ cases (Animal) a: | |
end | ||
} | ||
|
||
@section{Boolean Expressions} | ||
|
||
@subsection{Ask Blocks} | ||
|
||
If you have an @tt{if} block containing a lot of conditionals (greater than 3 | ||
branches), you should convert it to an @tt{ask} block. This aligns your | ||
conditionals along the left side of the block, which makes your code look | ||
cleaner and more readable. | ||
|
||
For example, this @tt{if} block is very verbose: | ||
|
||
@codedisp{ | ||
fun is-this-noah(str :: String) -> String: | ||
if str == "Noah": | ||
"Yes" | ||
else if str == "Nosh": | ||
"Close" | ||
else if str == "No": | ||
"ah" | ||
else if str == "Noa": | ||
"I don’t think so" | ||
else: | ||
"No" | ||
end | ||
end | ||
} | ||
|
||
However, we can make it more concise using an @tt{ask} block: | ||
|
||
@codedisp{ | ||
fun is-this-noah(str :: String) -> String: | ||
ask: | ||
| str == "Noah" then: "Yes" | ||
| str == "Nosh" then: "Close" | ||
| str == "No" then: "ah" | ||
| str == "Noa" then: "I don’t think so" | ||
| otherwise: "No" | ||
end | ||
end | ||
} | ||
|
||
@subsection{Concise Conditionals} | ||
|
||
You should never determine the boolean value of something by doing: | ||
|
||
@codedisp{ | ||
if some-boolean == true: | ||
... | ||
else: | ||
... | ||
end | ||
} | ||
|
||
Instead, you should use: | ||
|
||
@codedisp{ | ||
if some-boolean: | ||
... | ||
else: | ||
... | ||
end | ||
} | ||
|
||
Similarly, do not use an @tt{if} expression to evalulate a predicate, and | ||
then return @tt{true} or @tt{false}. For example, this would be | ||
considered incorrect: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this is about style, I feel it's weird to say that something is incorrect. Perhaps use "inappropriate" (or another word) instead? |
||
|
||
@codedisp{ | ||
if my-expression: | ||
true | ||
else: | ||
false | ||
end | ||
} | ||
|
||
Instead, the entire @tt{if} expression above can be replaced with | ||
@tt{my-expression}. | ||
|
||
@section{Naming Intermediate Expressions} | ||
|
||
@subsection{Local Variables} | ||
|
@@ -320,4 +429,4 @@ like JavaScript, you might think this is @emph{good} practice. It's | |
not: @emph{don't do this}! In Pyret, adding @code{var} turns each name | ||
into a @emph{mutable variable}, i.e., one that you can modify using an | ||
assignment statement. Therefore, do not use @code{var} unless you | ||
absolutely mean to create a mutable variable. | ||
absolutely mean to create a mutable variable. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad wording -- you just got through saying you don't want to have a list of Any type, but it's ok now to have a list of any type? (Note the capitalization...) You mean to say something closer to "Now, we don't know or care what the type of the elements of
lst
are, but we are asserting that they all be of the same type -- namelyA
. This distinction (between "any type at all", and "any single type, for any particular list") is subtle but important."There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworded it according to your comments—let me know what you think!