-
Notifications
You must be signed in to change notification settings - Fork 34
Developer Code Style Guide
Linting is provided by eslint. Install a plugin for your editor of choice to view the linting errors. Linting rules are enforced and your code must past the linter to be merged in. Feel free to discuss lint rules you encounter that seem incorrect or faulty - the rules may evolve over time.
For functions that return values consider using one of the following:
-
createThing()
- Create and return a new structured object (such ascreateEvent
which creates a new event object orcreateAttemptResponse
which creates a new attempt response object) -
getThing()
- Retrieve or pluck something from some data source (such asgetEvent
which retrives an event from the database orgetAttemptStatus(attempt)
which returns a status property from a passed in attempt object)
-
isSomething
- Boolean flags - all boolean flags should be prefixed withis
. -
someEl
- DOM or JSX element reference. All element references must be postfixed withEl
.
For your variables
- Use
const
whenever possible. - Otherwise, use
let
when you need to assign a new value to the variable - Never use
var
(There is a difference in scope to be aware of: let
is scoped to the nearest closing block while var
is scoped to the nearest function block. More info)
Use multi-line declarations instead of comma-separated declarations
// do this
let foo
let bar
// don't do this
let foo, bar
- If it's possible that a variable won't be set or that a function could not return an expected value
null
should be set or returned. The idea is thatnull
is an intentional value that means that something was intentionally not set or was intentionally not found.undefined
in comparison would mean that something was unintentionally not found. No value or function should set or returnundefined
, and if this does happen it should be expected to be a bug.
Don't do this:
let foo = getFoo()
getFoo() {
let myValue // myValue is not initalized (it is undefined)
...
return myValue // may return undefined if myValue is never set
}
Or this:
let bar = getBar(x)
getBar(a) {
if(!a) return // returns undefined
...
}
Instead, Do this:
let foo = getFoo()
let bar = getBar(x)
getFoo() {
let myValue = null // Initially set to null
...
return myValue // if myValue is never set in the hidden code above it will return null
}
getBar(a) {
if(!a) return null // Could also return some type of error, but never `undefined`
...
}
Just to be clear - You don't have to return null if the function return is not being used. This is fine:
onClick(event) {
if(!someEl.contains(event.target)) return // `return null` would be unnecessary!
...
}
- Use functional stateless components over React Classes whenever possible
- Functional stateless components should always be named, for example:
const NamedFunction = props => { ... }
export default NamedFunction
Use arrow functions (() => { ... }
) when possible instead of the older function() { ... }
syntax. There is a difference in scope between these two syntaxes to be aware of.
If/else statements for variable assignment should be avoided if a small in-line assignment or ternary could be used instead. For example:
Don't do this:
let isCorrect
if(score === 100)
{
isCorrect = true
}
else
{
isCorrect = false
}
Do this:
let isCorrect = score === 100
Don't do this:
let className
if(score === 100)
{
className = 'is-correct'
}
else
{
className = 'is-not-correct'
}
Do this:
let className = score === 100 ? 'is-correct' : 'is-not-correct'
But don't nest ternaries!
Don't do this:
let className = score === 100 ? 'is-correct' : (score === 0 ? 'is-not-correct' : 'is-partially-correct')
One ternary per line, please.
And don't do this:
let className
if(score === 100) {
className = 'is-not-correct'
} else {
if(score === 0) {
className = 'is-not-correct'
} else {
className = 'is-partially-correct'
}
}
The nested if/else adds complication to readability and is not needed.
This is better:
let className
if(score === 100) {
className = 'is-correct'
} else if(score === 0) {
className = 'is-not-correct'
} else {
className = 'is-partially-correct'
}
However, switch statements are generally preferred over if/else statements when testing for a single condition. Since the code above is only testing for score
we can rewrite this using a switch:
This is best:
let className
switch(score) {
case 100:
className = 'is-correct'
break
case 0:
caseName = 'is-not-correct'
break
default:
caseName = 'is-partially-correct'
break
}
If/else statements should be reduced to their simplest form. For example, this
if(a && b) {
if(c) {
// 1
} else if(d) {
// 2
}
} else if(b) {
// 3
}
Could be transformed into
if(!b) return
if(a && c) {
// 1
} else if(a && d) {
// 2
} else {
// 3
}
or
if(a && b && c) {
// 1
} else if(a && b && d) {
// 2
} else if(b) {
// 3
}
If you are returning a value in the example above, an even better version would be
if(!b) return
if (a && c) {
return 1
}
if (a && d) {
return 2
}
return 3
Base cases or exceptional cases should (when possible) be at the top of a function and should be handled with an early return rather than inside an if/else.
Example: Don't do this:
if(status !== null) {
// ... do something
} else {
return false;
}
And don't do this:
if(status !== null) {
// ... do something
}
return false;
Do this:
if(status === null) return false;
// ... do something
Here we check for required conditions at the top of a function and return early if they are not met. Should all required conditions be met then the main logic of the function is executed. This makes it easier for a developer to see the expectations of a function and to see the main logic of the function without having to parse a large if/else.
To aid in readability add in new lines to separate logical blocks of code, for example:
Don't do this:
onClick(event) {
if(!event.target.contains(someEl)) return
let newCounter = this.state.counter + 1
Dispatcher.trigger('clicked', newCounter)
this.setState({ counter:newCounter })
}
Do this:
onClick(event) {
if(!event.target.contains(someEl)) return
let newCounter = this.state.counter + 1
Dispatcher.trigger('clicked', newCounter)
this.setState({ counter:newCounter })
}
When the state of something can only be determined by multiple boolean flags then consider changing this representation to a single value (which can be set to multiple possible constants).
For example (shown with JSX but this is still applicable outside of React components), Don't do this:
<MyComponent isPractice={...} isAssessment={...} />
Here MyComponent
has two flags to determine what state it is in. If the intention is that only one of these values is true (for example <MyComponent isPractice={true} isAssessment={false} />
) then it is not clear what state would be represented by <MyComponent isPractice={true} isAssessment={true} />
). Your code may enforce that one of these values is always true but that would not be clear to another developer without comments or parsing your code, and a regression bug may creep in that removes this constraint. Additionally, if you needed to add another possibility to your component you would need to add another flag, and this would require reading and enforcing the state of three flags.
To avoid this problem, change this from multiple boolean flags to a mode
or type
property, such as
<MyComponent mode={...} />
mode
could be a string of either practice
or assessment
. These should then be put into constants, such as MYCOMPONENT_TYPE_PRACTICE
or MYCOMPONENT_TYPE_ASSESSMENT
. This way it is impossible to get into an unexpected state. Should another "mode" be added it would be trivial to add another constant and add another case
to a switch
statement.
When possible functions should not return JSX as these are easier to test. For example:
Don't do this:
let getMessage = (mode) => {
switch(mode)
{
case 'failed': return <p className="message">You failed</p>
case 'passed': return <p className="message">You passed!</p>
case 'not-taken': return <p className="message">No attempt taken</p>
}
}
...
render() {
return <div>
{getMessage(mode)}
</div>
}
Do this:
let getMessage = (mode) => {
switch(mode)
{
case 'failed': return 'You failed'
case 'passed': return 'You passed!'
case 'not-taken': return 'No attempt taken'
}
}
...
render() {
return <div>
<p className="message">{getMessage(mode)}</p>
</div>
}
Prefer to use the newer ES6 shorthand notation when possible. For example:
Don't do this:
{
user: user,
draft: draft
}
Instead, Do this:
{
user
draft
}
To aid in testability and readability it is better to have multiple smaller functions then one large function. When possible functions should perform a single task rather than multiple tasks. Large tasks can be performed by stringing together multiple functions. For example:
Don't do this:
updateScore(score, attempt) {
// insert score to database
db.query(`INSERT INTO scores(score) VALUES($[score])`, {score})
// create score event
let event = {
score,
userId: currentUser.id,
attemptId: attempt.id,
...
}
// insert score event to database
db.query(`INSERT INTO events(event) VALUES($[event]), {event})
}
Testing this method requires testing three things:
- Is the score inserted correctly?
- Is the event created correctly?
- Is the event inserted correctly?
Instead, Do this:
updateScore(score, attempt) {
insertScore(score)
insertEvent(createEvent(score, attempt))
}
Here we only need to test that insertScore
, insertEvent
and createEvent
were called as expected. We can then develop additional tests for insertScore
and the other methods.
Ideally, function names should be clear and code should be self-documented when possible. Additionally code comments are always recommended and preferred with one exception - if the code comment is made redundant by the code it is commenting it should be omitted.
Some examples:
Don't do this:
getData(arr) {
let values = {
largest: null,
highest: null,
lowest: null
}
if(arr.length === 0) return values
...
}
// set score to largest
let score = getData().largest
It is not clear what type of data
is returned from getData
. arr
is a non-helpful variable as it only tells us that we expect an array and not what that array should contain. Additionally, the comment doesn't provide any additional information that cannot be understood from the code itself.
Instead, Do this:
getAttemptsScoreSummary(attempts) {
let scoreSummary = {
largest: null,
highest: null,
lowest: null
}
// Return default summary if no attempts given
if(attempts.length === 0) return scoreSummary
...
}
let largestAttemptScore = getAttemptsScoreSummary(attempts).largest
These name changes add clarity to what the function does and expects. The comment isn't critical but does add some context to the intention that we are returning a "default summary".
A cute term for a conditional statement where the terms are in reverse logical order.
Example: Don't do this:
if(1 <= nodes.size) { ... }
Instead, Do this:
if(node.size >= 1) { ... }
Duplicate code will bite you (or another developer) as it would be easy to forget or miss that a piece of code you are updating has a counterpart that also needs updating. Duplicate code should be moved into its own function or the code should be reorganized so that duplicate code is no longer needed.
Example: Don't do this:
if(some.thing !== null) {
let myThing = getThing(some.thing)
myThing.init()
myThing.color = 'red'
}
else if(example !== null) {
let myThing = getThing(example)
myThing.init()
myThing.color = 'red'
}
return myThing
Instead, Do this:
f() {
//...
if(some.thing !== null) {
return createARedThing(some.thing)
}
else if(example !== null) {
return createARedThing(example)
}
return null
}
createARedThing(id) {
let myThing = getThing(thingId)
myThing.init()
myThing.color = 'red'
return myThing
}
Or, Do this:
let thingId = null
if(some.thing !== null) {
thingId = some.thing
} else if(example !== null) {
thingId = example
}
if(!thingId) return null
let myThing = getThing(thingId)
myThing.init()
myThing.color = 'red'
return myThing
Tests are one possible exception where repeating yourself is not a sin (for example, testing a function requires lots of duplication of calling the function with different parameters).
Using @TODO comments are fine during development but pull requests and finalized code should not include any of these comments. If the @TODO hasn't been addressed consider making an issue instead.
Example: Don't do this:
//@TODO - MOVE THIS method somewhere else!
StyleableText.createFromElement = () => { ... }
- Use
test
, don't useit
- This leads to better test names. For example:
Don't do this:
it('should add two numbers', () => {
expect(add(2, 2)).toBe(4)
})
Instead, Do this:
test('adds two numbers', () => {
expect(add(2, 2)).toBe(4)
})
Test descriptions should start with the name of the function being tested, followed by a brief description of the case that is being checked. For example:
Don't do this:
test('should add two numbers', () => {
expect(add(2, 2)).toBe(4)
})
test('add', () => {
expect(add(2, 2)).toBe(4)
})
Instead, Do this:
test('add should add two numbers', () => {
expect(add(2, 2)).toBe(4)
})
Special Cases
-
render
andonMount
methods for Obojobo Components are not explicitly tested. Instead, they SHOULD be tested using the[ComponentName] component
tests - Methods within a functional Obojobo Component MUST be tested indirectly in
[ComponentName] component
tests, as they are not accessible to outside callers
All tests for a function should be grouped consecutively within the test document, and test order should match the order that the functions appear in the original document. For example,
Don't do this:
// original file
method1()
method2()
method3()
// test file
test('method3 works', () => {})
test('method1 works', () => {})
test('method2 works', () => {})
Instead, Do this:
// original file
method1()
method2()
method3()
// test file
test('method1 works', () => {})
test('method2 works', () => {})
test('method3 works', () => {})
Furthermore, all tests for an Obojobo file should be contained within a single test file named [filename].test.js
, and wrapped in a describe
that is labeled with the name of the module. This file should be located in a directory that matches that original, inside of the test folder. For example,
Don't do this:
|- file1.js
|- file2.js
|- folder1
|- file3.js
|- __tests__
|- file1.js
|- file3.test.js
|- test2.test.js
Instead, Do this:
|- file1.js
|- file2.js
|- folder1
|- file3.js
|- __tests__
|- file1.test.js
|- file2.test.js
|- folder1
|- file3.test.js
When mocking an id it's preferred to use a string such as mock-draft-id
instead of a number. For example,
Don't do this:
Draft.fetch.mockResolvedValueOnce({ id:123, ... })
Instead, Do this:
Draft.fetch.mockResolvedValueOnce({ id:'mocked-draft-id', ... })
Prefer to use mockReturnValue
or mockReturnValueOnce
over returning values with mockImplementationOnce
or mockImplementationOnce
when the implementation only returns a value. For example,
Don't do this:
myMockedFunction.mockImplementation(() => {
return 'mocked-value'
})
Instead, Do this:
myMockedFunction.mockReturnValue('mocked-value')
Prefer to use mockResolvedValue
and mockResolvedValueOnce
over returning a promise with mockReturnValue
or mockReturnValueOnce
. For example:
Don't do this:
myAsyncMockedFunction.mockImplementationOnce(() => {
return new Promise((resolve, reject) => {
resolve('mockedValue')
})
})
or
myAsyncMockedFunction.mockReturnValueOnce(new Promise((resolve, reject) => {
resolve('mockedValue')
})
Instead, Do this:
myAsyncMockedFunction.mockResolvedValueOnce('mockedValue')
Style linting is provided by stylelint. Install a plugin for your code editor of choice to view the linting rules in your editor. Linting rules are enforced and your code must past the linter to be merged in. Feel free to discuss lint rules you encounter that seem incorrect or faulty - the rules may evolve over time.
- kebab-case is required for all selectors
-
is-*
-is
indicates a style which is applied programmatically via javascript.- Boolean properties: Programmatic classes for boolean properties (for example
is-correct
oris-enabled
) must include a matchingis-not
rule and either theis-
oris-not
rule must always be applied. For example, if a component may add anis-correct
class to an element then it must add anis-not-correct
class in cases where theis-correct
class is not added. - Non-boolean properties: Programmatic classes for properties that are non-boolean (for example, a
mode
property which may be"practice"
or"assessment"
) should always be applied and should use the syntaxis-mode-*
(oris-type-*
or some other similar identifier). In this example, these would beis-mode-practice
oris-mode-assessment
.
- Boolean properties: Programmatic classes for boolean properties (for example
Whenever possible use em
units. px
units are not allowed except for minor translate
methods and the size of borders
.
(Our standards are loosely inspired by rscss which is an opinionated style guide similar to BEM.)
- Components must use a namespace that is unique and mirrors the namespace of the React component. Some examples include
obojobo-draft--viewer--components--dialog
(for ObojoboDraft.Viewer.components.Dialog) andobojobo-draft--chunks--question
(for ObojoboDraft.Chunks.Question). - Rules should be nested inside a class selector for a component.
- Whenever possible use the > child selector. The use of child selectors makes refactoring more difficult but the tradeoff is that styles do not bleed through nested components. Since Obojobo documents are collections of reusable nested components it is critical that the styles of components do not modify any children components and child selectors accomplish this. More info. An example:
Don't do this:
.obojobo-draft--components--my-component {
select { /* All `select` elements inside this component will now be absolutely positioned! */
position: absolute;
}
}
Do this:
.obojobo-draft--components--my-component {
> select {
position: absolute;
}
}
Write SQL with uppercase letters with every major token on a new line.
Don't do this:
select * from users where id = '6'
Do this:
SELECT *
FROM users
WHERE id = '6'