Developer Code Style Guide

Javascript

Linting

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.

In some cases you may need to ignore the linter (typically in tests) - you can do this in eslint in a few ways:

Do this:

// eslint-disable-next-line no-console
expect(console.error).not.toHaveBeenCalled()

Or, do this:

getFirst.mockReturnValue(undefined) //eslint-disable-line no-undefined

Don't do this:

// (Beginning of file)
/* eslint-disable no-undefined */
...

In general using in-line eslint comments are recommended. However you can use the global file-level disable in tests if using in-line comments would result in an excessive amount of comments!

Function naming conventions

For functions that return values consider using one of the following:

createThing() - Create and return a new structured object (such as createEvent which creates a new event object or createAttemptResponse which creates a new attempt response object)
getThing() - Retrieve or pluck something from some data source (such as getEvent which retrives an event from the database or getAttemptStatus(attempt) which returns a status property from a passed in attempt object)

Variable naming conventions

isSomething - Boolean flags - all boolean flags should be prefixed with is.
someEl- DOM or JSX element reference. All element references must be postfixed with El.

`var`, `let` and `const`

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

Initial value of variables, use of `null` and `undefined`

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 that null 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 return undefined, 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!
  ...
}

React Components

Use functional stateless components over React Classes whenever possible
Functional stateless components should always be named, for example:

const NamedFunction = ({propA, propB}) => { ... }
export default NamedFunction

Arrow functions

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.

Prefer destructured props

When passing props to a functional component, its cleaner to define them in the method as a destructed object rather than access them from the props object.

Example: Don't do this!

const Results = (props) => <div>{`${props.userName} your score is ${props.score}`}</div>
export default Results

Instead, do this:

const Results = ({userName, score}) => <div>{`${userName} your score is ${score}`}</div>
export default Results

Ternary vs If/else vs Switch

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

If/else logic & early returns

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.

Newlines

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 })
}

Flags

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 to return JSX

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>
}

Simplifying objects with ES6 shorthand notation

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
}

Functions & Testability

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.

Code comments & function names

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".

No "Yoda Conditions"

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) { ... }

DRY - Don't repeat yourself

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).

Avoid @TODO comments

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 = () => { ... }

Jest Unit-Tests

`test` vs `it` and test names

Use test, don't use it - 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

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 and onMount 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

Test File Organization

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

Mocked ids

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', ... })

Mocking return values vs implementations

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')

Mocking promises

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')

CSS (Sass)

Linting

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.

In some cases you may need to ignore the linter - you can do this in stylelint in a few ways:

Do this:

// stylelint-disable-next-line unit-blacklist
border: 1px solid #b3b3b3;

Or, do this:

max-width: calc(100% - 10px); //stylelint-disable-line unit-blacklist

Don't do this:

/* (Beginning of file) */
/* stylelint-disable unit-blacklist */
...

Selectors

Classes are always preferred over ids since ids are "global".

Do this:

<div className="equation-container">

Don't do this:

<div id="equation-container">

If you must use an id it should be uniquely named in order to minimize the potential to have two or more elements on the page with the same id.

Do this:

<div id="obojobo-draft--my-components--dialog">

Don't do this:

<div id="dialog">

Class naming conventions

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 or is-enabled) must include a matching is-not rule and either the is- or is-not rule must always be applied. For example, if a component may add an is-correct class to an element then it must add an is-not-correct class in cases where the is-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 syntax is-mode-* (or is-type-* or some other similar identifier). In this example, these would be is-mode-practice or is-mode-assessment.

Units

Whenever possible use em units. px units are not allowed except for minor translate methods and the size of borders.

Component styles

(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) and obojobo-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;
  }
}

SQL

Syntax

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'

Obojobo is a UCF open source project.

Documentation || Obojobo on Github || About UCF Open

Developer Code Style Guide

Javascript

Linting

Function naming conventions

Variable naming conventions

var, let and const

Initial value of variables, use of null and undefined

React Components

Arrow functions

Prefer destructured props

Ternary vs If/else vs Switch

If/else logic & early returns

Newlines

Flags

When to return JSX

Simplifying objects with ES6 shorthand notation

Functions & Testability

Code comments & function names

No "Yoda Conditions"

DRY - Don't repeat yourself

Avoid @TODO comments

Jest Unit-Tests

test vs it and test names

Test Descriptions

Test File Organization

Mocked ids

Mocking return values vs implementations

Mocking promises

CSS (Sass)

Linting

Selectors

Class naming conventions

Units

Component styles

SQL

Syntax

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Clone this wiki locally

`var`, `let` and `const`

Initial value of variables, use of `null` and `undefined`

`test` vs `it` and test names