Draft guidance on code readability

[ZoeBijl]

Our code should be as easy to read as possible. People should be able to jump into it and understand it straight away. To achieve this we’ll need simpler code. And to achieve that we’ll need guidance on code readability that we can point to.

---

Proposal topics

This is a list of topics/rules around which I want to draft this guidance. Will elaborate/explain these later. This is just a brain dump at the moment.

• No unnecessary comments for documentation tools or code editors
• No unnecessary comments at all
• No explaining how something works, this is not a tutorial
• Fine to have comments that explain what a line of code does (ex: trigger collapse action when return is pressed)
• easy function notation, none of this dot notation stuff
• HTML should use as few classes as possible
• HTML should use as few ARIA as possible (meaning, use a button-element if usage of the button-role is otherwise irrelevant to the example)
• CSS should be easy to follow meaning there should be some logic behind the order of declaration groups
• Don’t need to enforce an order of CSS properties as that’s very arbitrary
• File names should be easy to read too and ideally there should only be one for each type per example
• Where possible differences in implementation should be done via attributes/settings rather than creating an entirely new JS file
• Be clear, not clever (if five lines are clearer than one line of clever code, go for the clear five line version)
• No “more” and “less” accessible version of a code example. We’re the APG, we should only have accessible stuff.
• Keep your code example in one JS file. No one wants to figure out which of the five provided files contains the bit they’re looking for.
• Dash notation for CSS classes: component-part not componentPart or component_part

Proposal guidance

Written out text for the above topics.

HTML

If you watch Effortless Style by Heydon and read my Cool Semantic Styles article and apply what is said in those you should be pretty close to what we want.

Resources

• Effortless Style by Heydon Pickering (slides)
• Cool Semantic Styles by Zoë Bijl

CSS

tbw

JS

tbw

[mcking65]

@ZoeBijl, please add any new guidance to this wiki page:

https://github.com/w3c/aria-practices/wiki/Code-Guide

[ZoeBijl]

@mcking65 I will add it to those wiki pages. But I’ll first draft things in this thread so they can be more easily discussed.

[jongund]

@ZoeBijl

In the examples I have developed I used prototype functions, so they would be easy to convert to using classes.

Menubar example

Radio Button Group example

The prototype functions are typically named for the ARIA role they correspond to, to help people see the properties and interaction of an ARIA role.

[smhigley]

@ZoeBijl thatnks so much for opening this, it looks great! Also +1 to classes and es6 syntax :)

Would you be interested in using prettier/https://github.com/prettier/prettier to help maintain a consistent (and likely familiar) js code style? I personally prefer it to eslint or jslint because:

• it's already opinionated, so we don't need to separately discuss what style rules to adopt. My impression is it has wide enough adoption and the rules are chosen carefully enough that the resulting code style is familiar to many developers
• it feels less like a gotcha when developing, since it automatically fixes formatting (on save, or build, or CI, depending on what we implement).

My other thought was it might be nice to include some guidance on good comments to add in addition to comments to avoid. I noticed there are some @param/@desc comments on a few examples, though not consistently -- are those the ones you'd like to remove as unnecessary and for docs tools/code editors? I don't have much of an opinion either way on those, although I personally like having human-readable comments that describe the type of parameters accepted and return value (if applicable). On the other hand, I also like Typescript, so take my opinions with a grain of salt 😂.

(small example, but maybe requiring this would be too much of a burden on authors. I also don't really have an opinion on the formatting/template)

/*
 * Function to determine if a given option is selected
 * @param option: HTMLElement
 * @returns boolean
 */
function isOptionSelected(option) {
    return true/false;
}

[ZoeBijl]

@jongund is this something we could automate? Happy to do it manually tho.

@smhigley I stan a thing that automatically fixes things on save/build. I think we currently use eslint tho? And we should totes include examples of good comments. As far as I know the @param things are mainly useful for IDEs. Since we’re not writing production code those seem unnecessary to me. If our code is unreadable without such comments then I think we should change our code. Meaning, it shouldn’t much matter what kinda thing is sent to a function for the reader to understand what the function does (for our example).

[jongund]

@ZoeBijl I used this type documentation pattern (e.g. @param and @returns ) in early examples I wrote and people thought it was more distracting than helpful, especially if the function and prototype names are descriptive as possible and having the code be as self documenting as possible. But like all groups they some times change their minds and I am open to other ideas, but less is more in most coding.

[smhigley]

I agree some of the @param and @returns can get too verbose and unnecessary. One thing I really appreciate from Typescript is being able to tell at a glance the types of the arguments and return value, which lets you see something and immediately know e.g. "oh, this isSameDay function accepts two Date or null arguments and returns a boolean."

I find it also makes code much easier to refactor, because you can do anything to the internals of a function so long as you still accept and return the same types, or be more confident updating other logic if you know e.g. "I need to always give this function either a Date object or a null value, but not undefined"

[sh0ji]

I don't disagree with anything in here really, but I'm not terribly concerned about what our opinions are as much as I am about how we can enforce and maintain them in a way that is both flexible and forgiving to would-be contributors. We're seeing feedback about outdated and unreadable code not because we don't have clear guidelines—the code guide covers quite a bit—but because simply having reference documentation isn't enough.

Echoing @smhigley but maybe with a bit more force, I feel rather strongly that we should outsource most of our code style to more opinionated and more well-backed styles that exist for each given language, and only maintain rules when we feel strongly that they should differ. Most modern code quality tools allow us to override existing rules so that we can effectively say, "use <some idiomatic style> but with tabs instead of spaces."

I like Prettier as well and I really like TypeScript, but I do question whether they are the right tools for this job. TypeScript seems like an especially big hurdle—even the JSDoc comment syntax (@param/@returns) seems like too much to me.

[sh0ji]

Here are some of the tools that I often use and some rulesets that I recommend for each:

• ESLint is a rule-based code linter for JavaScript. It doesn't come with any rules enabled, but there's a very well-supported ecosystem of shareable configs.
  • eslint-config-airbnb-base is my preferred base ruleset for ES6 syntax. It's well maintained, and its strong opinions are extensively documented. It can also be customized quite a bit, including omitting the Node rules when you want to use it in a browser-only project like this one.
  • eslint-config-prettier is an ESLint implementation of Prettier's opinions. It's great, but not quite as opinionated as airbnb's. Of course, there's nothing stopping you from combining it with airbnb-base or another config if you like.
  • eslint-plugin-compat allows you to use browserslist to define your browser support and it will flag WebAPIs/syntax that aren't supported. Useful if we don't want to transpile our JavaScript.
• Stylelint works almost identically to ESLint, but for CSS and its derivatives (Sass, Stylus, LESS...).
  • stylelint-config-standard is what we're currently using without any modifications. It references @mdo's code guide, which is also what our own code guide references, so it seems well-suited here.
• Prettier describes itself as a code formatter rather than a linter. It can kind of function as a linter via the --check flag in the same way that ESLint/Stylelint can function as a formatter via the --fix flag.
  • Prettier isn't meant to be extendable in the way that linters are, meaning we would be more tied to their opinions than we would if we used ESLint/Stylelint.
  • It's great for formatting HTML, Markdown, JSON, YAML, etc. to ensure consistency and with reasonable opinions. None of those languages have great linters, so I tend to use Prettier for them.
  • I still prefer eslint/stylelint for js/css, respectively.

[smhigley]

I'm not super opinionated on linters, or even on code style, as long as there's one consistent rule set for the entire codebase. I think my only two preferences would be:

• Pick something existing like ESLint/Stylelint or Prettier, so we don't need to maintain a complete set of code style rules ourselves (just echoing what was already said)
• I really like automatically fixing errors over highlighting them for authors or catching them in a failing build. I haven't been having great luck with eslint --fix in aria-practices, but perhaps it would do better with es6 rules? (at least there seem to be more wrenches there: https://eslint.org/docs/rules/)

I agree with @sh0ji on Typescript as well -- as much as I like it, I don't think it's a great fit here. If there's some author-friendly way of noting down argument and return types I think that would capture some of the benefits of typescript, though it's quite manual and may degrade over time and with many authors.

Another possibility might be to use Flow with no explicit type annotations -- it would provide in-editor type hints and type checking but without sacrificing vanilla JS syntax

[sh0ji]

We could write TypeScript declaration files after-the-fact without touching the original JavaScript if someone's feeling ambitious...

The current eslint rules are rather minimal, which is in fact why --fix doesn't do too much. stylelint --fix did quite quite a lot when I ran it a while back (b552069). You may also not notice some of the things that are getting fixed because both stylelint and eslint run with --fix on staged files when you make a commit.

@nschonni, not sure if you saw this thread but I thought you may be interested!

[nschonni]

Taking a look to see how much needs to change for the ESLint "extends": "eslint:recommended" or "extends": "airbnb-base"

[nschonni]

Took a shot with the "eslint:recommended" in #1207 and it looks like it actually caught a few things that could be bugs. There are ~250 unresolved issues for unused and undefined variables that I just ignored for now.

[ZoeBijl]

Thank you all for the discussion, there’s some interesting things in here. For me though, I think most things I’d like to see changed aren’t really things a linter/formatter would catch. How for example would either of those catch whether a function name is readable or makes sense? How would those catch unneeded classes? How would those check if the order of CSS declaration blocks makes sense?

To me automation isn’t the solution for this issue. This will require human interpretation of the code. Which is kinda fine since we only have 27 design patterns. Sure there are some variation. But even if we totalled 40 code examples that would still be fine.

May I suggest we move further linter/formatter rules to another issue? Or would yinz prefer we keep that in this thread since there’s already quite a bit of information here?

[sh0ji]

I fully agree that we should focus on the human review. I just feel strongly that a big part of doing that is to automate as much as possible, allowing us humans to focus on the tougher aspects of code design, like naming things.

It's not always clear what can be automated and what can't, which is an argument for waiting to split off the automation discussion. For instance, you can ensure that CSS declaration order always increases in specificity with stylelint/no-descending-specificity, which is how I read "the order of CSS declaration blocks makes sense." That's a rule that's currently enabled in this repo, in fact.

[ZoeBijl]

I rather meant the order of the declaration blocks, not the properties within them.

And I’m all for automated things. I would like this thread focused on the human aspect 👍.

[sh0ji]

I rather meant the order of the declaration blocks, not the properties within them.

That's what the Stylelint rule I referenced addresses.

[ZoeBijl]

That can’t look at the DOM order to see if it makes sense / matches up. I’d rather not automate something a human is much better at.

[spectranaut]

Here is an attempt to summarize the discussion on this PR about best practices for example code:

The purpose of the example code is to provide an easy to read implementation of the JavaScript logic necessary to correctly set aria attributes on an interactive widget. Using best practices for code design in a production environment is not always applicable in this case.

All of an example widgets’ logic should be contained in a single class with rare exceptions. For example, if a component in one example widget would best be used as is in another example widget. If the widget has been split up into multiple classes for this reason, communication between classes should be done via events.

@ZoeBijl do you want to take the relevant feedback and your original suggestions and write a draft? Do you want more feedback on anything?

[a11ydoer]

@maschad96 Welcome to the APG team. This is your first issue.

[maschad96]

Hi all, I’ve drafted a small markdown document simply to start some discussion, everything noted in it is up for discussion and nowhere near final. 👇

HTML

Naming

Attribute Usage

---

CSS

Naming

Attribute Usage

---

JavaScript

Naming

1. Numbering should start at 0 in any case where ordinals are defined, for consistency and avoiding conditions such as i <= n + 1 or i <= n - 1 where possible
2. Functions that return booleans should be prefixed with is, has, are or similar keywords distinguish the return value, (e.g: isVisible, areEqual, hasEncryption)
3. Functions that return a value aren't always obviously getting or posting information, so it's good to prefix function names with verbs:

   - function buttonLabel(label) {
   -	...
   - }
   + function applyButtonLabel(label) {
   +	...
   + }

4. Be specific in naming, for example: allowMultiple may require a comment to explain. allowOpeningMultipleAccordions is long, but I understand what it does when reading through if logic

Structure

1. Widgets logic should be stored within its own class, and communication between separate components should happen via an Event Bus.
2. Where functions are taken as argument, having them predefined and passed in better communicates the functions purpose as an argument
3. Avoid double-barrelled functions, for example setFocusAndDoStuff. Calling setFocus and doStuff from handleFocus explains the logic well when viewing handleFocus definition. Ideally this reduces refactor thoughts about the callee's name
4. Similar to Clean up “common” folder #3 in purpose, long methods should be broken into smaller methods so contributors can a) have more clear direction to where their interest is occurring and b) better segment errors into their small methods for troubleshooting
5. Replace nested conditionals with guard clauses:

   - function getResult() {
   -   let result;
   -   if (A) {
   -     result = resultA();
   -	} else {
   -     if (B) {
   -       result = resultB();
   -     } else {
   -       if (C) {
   -         result = resultC();
   -       } else {
   -         result = resultD();
   -       }
   -     }
   -   }
   -   return result;
   - }
   -
   + function getResult() {
   +   if (A) return resultA();
   +   if (B) return resultB();
   +   if (C) return resultC();
   +   return resultD();
   + }

6. Avoiding temporary fields, such as bloated objects being passed and only cherry-picking one or two properties
7. Classes and possible changes should have a one-to-one relationship, changes shouldn't need to cascade into separate classes

Comments

1. Comments shouldn't explain "what" something is doing, but rather the "why". If the "why" is obvious through method name and nested calls, it may not need a comment.
2. No JSDocs comments or anything, only comments as necessary
3. If a comment is necessary, it should precede the code being explained

   - function socialize() { //calls a function
   -   if (!friendIsHome || !isFeelingExtroverted) { // checks if friend isn't home or if i'm not extroverted
   -     return;
   -   }
   - 	return startConversation() // chat with friend
   - }
   -
   + // If my friend is home and I'm feeling extroverted, start a conversation
   +
   + function socialize() {
   +   if (!friendIsHome || !isFeelingExtroverted) {
   +    return;
   +   }
   +  return startConversation();	
   + }

[spectranaut]

Hey @maschad96 -- this is such a great start, and in line with the direction we are already going in and somethings we've discussed in the past.

I have a few suggestions about where to go from here, I'm hoping you can carry this project through :)

• Could you add code examples, or maybe links to a blog post, for Structure.1 "Event bus" and Structure.2 "Function passing"
• @smhigley recent PR for the select only combobox (Select-only combobox #1396) is a good example of clean and readable code with good structure. I think it would be good to reference this example in the code guide wikipage. There are few if any departures from your suggestion, could you do a read through it, and open a PR to change anything that should be changed to match your suggestions here? You might see some conventions she uses and copy them over into the draft you have here as well.
• After step two, copy the conventions over into the wiki! :)

[jongund]

What is the though about external helper functions in examples, like select only example?

I have included helper functions as a part of the prototype:

• Carousel with tablist example
• Menubar editor example
• Datepicker Dialog example

Also what about the use of case statements in the keyboard event handler to make it easy to see which keys are used in a particular pattern example. I feel people being able to easily identify the keyboard support in the code should be a top priority of the APG examples, since this is one of the biggest problems is keyboard support when people try to using ARIA.

[smhigley]

Sorry for the delay, I was trying to finish some wiki updates (in a branch) to preview before the meeting, but for now here's an outline of what I was thinking (with @maschad96's work integrated):

General Code Style & Syntax

Please refer to the AirBnB Javascript Style Guide for all basic javascript syntax and code style rules.

Function Naming

1. Functions that return booleans should be prefixed with is, has, are or similar keywords distinguish the return value, (e.g: isVisible, areEqual, hasEncryption).
2. Functions that return HTML snippets should be prefixed with render, for example:

function renderButton(label) {
  return `<button type="button">${label}</button>`;
}

1. Functions that are intended to be event handlers should be prefixed with on, e.g. onKeyDown. If the event handler is specifically for one element within a complex widget, include the element name as well: onButtonClick.
2. Functions that mutate some form of state should be prefixed with update, or a verb that describes the state change. The full function name should be in the format of "verb + name of state" For example, openListbox or updateFilterString.
3. Functions that return a value aren't always obviously getting or posting information, so it's good to prefix function names with verbs:

   - function buttonLabel(label) {
   -	...
   - }
   + function applyButtonLabel(label) {
   +	...
   + }

4. Be specific in naming, for example: allowMultiple may require a comment to explain. allowOpeningMultipleAccordions is long, but I understand what it does when reading through if logic

Function Design

1. whenever possible, write "pure" functions that do not have external effects, and do not depend on anything apart from the inuts (i.e. given the same input, the function will always return the same value. And it will not have side effects)
2. if a function does have side effects, those MUST be detailed in a comment before the function, or the function MUST be named "updateX" where its primary purpose is to update X state.
3. I'd put what Matt wrote about function design under "Structure" here -- i.e. that functions should be single-purpose, nested logic broken down, etc.

Comments

• (what Matt wrote)
• comment formatting: use airbnb styles
• don't add a comment that only duplicates a variable or fn name (this is currently part of the APG style guide)

Widget Structure

• What Matt wrote
• Would like to standardize some other basic formatting, e.g. functions within a class should be in alphabetical order, state variables are grouped at the top, constants are grouped at the top, helper fns go inside (or outside) the class

[nschonni]

Opened #1553 with to show what the change would be to use primarily the Prettier defaults with the existing ESLint setup

[nschonni]

Opened #1554 to show the Prettier defaults for CSS, based off the existing Stylelint config.

I've got HTML and MD over in a combined branch https://github.com/nschonni/aria-practices/tree/prettier, but it caused an issues with 2 tests, so stopped with submitting the JS and CSS for now

[smhigley]

I wasn't able to find a good way to create a PR or preview link for the wiki's git setup, so I put the new file in a markdown gist here: https://gist.github.com/smhigley/2064dc84b5bbd2b653f2d8a6ed87ae60.

Could anyone interested leave reviews/comments directly on the markdown file? The changes include the entire "Javascript" section, and nothing else.

[nschonni]

Would it make sense to put that in the https://github.com/w3c/aria-practices/blob/master/CONTRIBUTING.md instead of the wiki?

[a11ydoer]

The group will add code documentation wiki link to contributing.md upon your suggstion, @nschonni

[smhigley]

Wiki updated :)

[nschonni]

@smhigley thanks! can you also fix the reference to npm run lint:fix, as it should be npm run fix without the lint: prefix (in https://github.com/w3c/aria-practices/wiki/APG-Infrastructure)

[a11ydoer]

@nschonni
Would you add code documentation wiki link to contributing.md once Sarah fix the reference or should I? Just let me know.

[nschonni]

@a11ydoer go for it! I haven't dug through the wiki much, so you probably know what links should be included

[smhigley]

@nschonni I just updated the npm run fix line, thanks for catching that :)

@a11ydoer thanks for doing the link update in the main repo 👍

[spectranaut]

This looks so great, thanks @smhigley ! Seems like it's time to close this issue :)