Skip to content

Updates to code layout and formatting #115

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

Merged
merged 1 commit into from
Nov 12, 2018
Merged

Updates to code layout and formatting #115

merged 1 commit into from
Nov 12, 2018

Conversation

vexx32
Copy link
Contributor

@vexx32 vexx32 commented Sep 30, 2018

  • Update document layout to follow general Markdown guidelines with respect to headers, code fencing, etc.
  • Add missing portions and replace TODOs that have been sitting around for aeons
  • Increase self-consistency with respect to examples
  • Improve existing examples and add one or two more.


#### Indentation
Having a script written in the order of execution makes its intent more clear. There is no functional purpose to having `begin` be declared _after_ `process`, and although it will still be executed in the correct order, it should be considered that writing in such a fashion significantly detracts from the overall comprehensibility of a script.
Copy link
Member

Choose a reason for hiding this comment

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

Very long sentence, please break up.

Remove "it should be considered that" (it's redundant). Try for a simpler wording of "writing in such a fashion" and "significantly detracts for the overall comprehensibility"


# This is also good
Copy link
Member

Choose a reason for hiding this comment

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

The point of this section about hanging indents or continuation lines is that it is ok to not use a single level 4-space indentation.

Adding an example that is according to the normal standard seems redundant, but it would at least need to be explicitly called out that it's here for contrast: "According to normal indentation rules, you would write it this way ... "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that I saw the need to add a 'normal' example, perhaps the intent needs clarification? ;)

I'll review and see what I can do.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it should be reworded to say more than earlier in the paragraph, and to be clearer about the exception cases. Maybe we could put a simple example of indented code before the second paragraph, and then change that paragraph to read like:

Use four spaces per indentation level

Usually you use the [Tab] key to indent, but most editors can be configured to insert spaces instead of actual tab characters when you indent. For most programming languages and editors (including PowerShell ISE) the default is four spaces, and that's what we recommend. As always, individuals, teams and projects may have different standards and you should abide by them...

function Test-Code {
    foreach ($exponent in 1..10) {
        [Math]::Pow(2, $exponent)
    }
}

Indenting more than 4-spaces is acceptable for continuation lines (when you're wrapping a line which was too long). In such cases you might indent more than one level, or even indent indent an odd number of spaces to line up with a method call or parameter block on the line before.

function Test-Code {
    foreach ($base in 1,2,4,8,16) {
        foreach ($exponent in 1..10) {
            [System.Math]::Pow($base,
                               $exponent)
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does look a lot clearer, thank you!


Use a single space after commas and semicolons, and around pairs of curly braces.

Avoid extra spaces inside parenthesis or square braces.
Avoid unnecessary extra spaces inside parenthesis or square braces.
Copy link
Member

Choose a reason for hiding this comment

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

Weakens the statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, perhaps. The original statement seems to contradict prior assertions, however, implying vaguely that all space that can be omitted inside such expressions should... Which i do not think is wise or consistent with prior assertions.

Copy link
Member

Choose a reason for hiding this comment

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

Well, frankly: you should not put spaces inside parentheses or square braces -- although it's sometimes acceptable when you have multiple parentheses together (particularly when it wraps across multiple lines).

I mean, for example, don't write:

foreach ( $x in Get-Content $file ) {
    if ( $Variable[ $x ] ) {

Instead write:

foreach ($x in Get-Content $file) {
    if ($Variable[$x]) {

What do you think of changing this to read:

Sub-expressions and scriptblocks, on the other hand, should usually have a single space on the inside of the braces or parentheses to make them stand out and improve readability. Note that of course, sub-expressions and variable delimiters inside strings should not include additional space, because it affects the actual string.

foreach ($x in $( Get-Content $file )) {
    Write-Host "The number [Math]::Pow(2, ${x}) = $( [Math]::Pow(2, $x) )"

Although after some performance testing, we've basically stopped writing ${x} unless we're using non-standard variable names, and are using $($x) when it's necessary to separate the variable from the text around it.

In fact, at work we've decided all variables in strings should be wrapped in a sub-expression, so that we can more easily refactor using search-and-replace in our editor without worrying about whether there's an instance in a string when we change $Thing to $Thing.Name or whatever.

Copy link
Contributor Author

@vexx32 vexx32 Oct 5, 2018

Choose a reason for hiding this comment

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

Well, for one the foreach ($x in $( Get-Content $file )) { is no different to foreach ($x in ( Get-Content $file )) { in general (in fact, most people I've spoken to have no idea what $() really adds outside of the standard usage in strings).

But overall I tend to agree, mostly. Excessive spacing in an expression can sometimes detract, though, as you mention, so there's a bit of a "do what works best for you" element here as well for some of the more complex expressions.

Example:

for (int $i = ($j - 1) ; ($i -gt 19) -or ($i -lt 1) ; ($i + ($j / 2)) { }

(Now granted, that is a highly contrived example, but the point is mainly that there are a great many places you could opt to place additional spaces -- but adding spaces everywhere makes the expression more difficult to read, because the expressions lose their grouped appearance which lends some intuitive clarity ("things that look like they're together work together" sort of idea).


Not sure about the $($x) vs ${x} myself. I too tend to use them only when necessary (e.g., ${c:\windows\drivers\etc\hosts}, but I find myself preferring them to delimit variables in strings.

It may also be worth a mention that some folks prefer to reserve that syntax for function parameters to distinguish them from purely local variables.

Copy link
Member

Choose a reason for hiding this comment

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

We actually standardized on $($x) over ${x} for two reasons:

  1. It appears that the sub-expression is actually faster (although this is hard to measure)
  2. It kept coming up that someone would try to refactor variables into objects and we'd end up trying to do a refactor of, say $userName to $User.Name -- you can rename $userName to $User.Name with a simple search and replace, but only if you never wrote ${userName} anywhere...

Not trying to put that in the best practices book (yet) though 😉


Nested expressions `$( ... )` and variable delimiters `${...}` inside strings do not need spaces _outside_, since that would become a part of the string.
Nested expressions `$( ... )` and variable delimiters `${...}` nested inside strings should not include additional space _surrounding_ them, unless it is desired for the final string to include them.
Copy link
Member

Choose a reason for hiding this comment

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

Adding "nested" a second time in this sentence is redundant: "Nested expressions ... nested inside strings"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems more as though 'nested expressions' was being used as the term to refer to $( ... ) rather than it being tested inside strings; subexpressions have uses outside strings as well.


```powershell
$Var = 'out'
"This is a string with${Var} unnecessary spaces."
Copy link
Member

Choose a reason for hiding this comment

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

This example is way too clever -- the reader has to parse it for the sentence to even make sense, and until they do, it's not at all clear what it's meant to be an example of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mm. I feel as though there ought to be some kind of example here, but tbh I'm a little grasping at straws for what, exactly, makes sense here. I'll think on it.

@Jaykul
Copy link
Member

Jaykul commented Oct 3, 2018

Overall I like these edits (although your changes will conflict with #117), but you can't change the header depth. This is just one section of a book and the headline levels need to make sense in the larger context.

@vexx32
Copy link
Contributor Author

vexx32 commented Oct 3, 2018

Ah, I see. That makes sense. I'll take care of that.

Thanks for the comments :)

If you elect to merge that one first I'll rebase and deal with any conflicts, no biggie :)

@Jaykul
Copy link
Member

Jaykul commented Oct 11, 2018

Sorry @vexx32, but since you volunteered ... please resolve

@vexx32
Copy link
Contributor Author

vexx32 commented Oct 12, 2018

@Jaykul All fixed, I do believe... Markdown's extra whitespace plays havoc with VS Code's diff tools 😆

Think I caught all the additions in the previous PR and rolled them in. 😄

@vexx32
Copy link
Contributor Author

vexx32 commented Oct 30, 2018

@Jaykul anything else I should fix here? 😄

@Jaykul Jaykul merged commit bb37adc into PoshCode:master Nov 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants