# JRNI Studio development style guide **Table of Contents** - [JRNI Studio development style guide](#jrni-studio-development-style-guide) - [Workflow](#workflow) - [Development practices](#development-practices) - [Opportunistic refactoring](#opportunistic-refactoring) - [Testing](#testing) - [Naming conventions](#naming-conventions) - [Variable names](#variable-names) - [Method names](#method-names) - [File names](#file-names) - [Git branch names](#git-branch-names) - [AngularJS Style guide](#angularjs-style-guide) - [AngularJS files](#angularjs-files) - [Prefer $ctrl over specific named controllers](#prefer-ctrl-over-specific-named-controllers) - [Use ES6 modules instead of AngularJS services](#use-es6-modules-instead-of-angularjs-services) - [Making a Pull Request](#making-a-pull-request) - [A Pull Request is the responsibility of the author](#a-pull-request-is-the-responsibility-of-the-author) - [A clear and descriptive title](#a-clear-and-descriptive-title) - [Fill out the template in the github editor](#fill-out-the-template-in-the-github-editor) ## Workflow The development work of a ticket should generally follow the same linear process: - Do work on feature branch using the ticket ID as the branch name. ed `git checkout -b SE-3000` - When ready for testing deploy the branch so that it may be tested. Run `npm run deploy -- --env development` which will generate a URL using the name of the branch. - Put the JIRA ticket you're working on into the QA status and provide the link to the deploy for testing. - Create a pull request and have the changes peer reviewed. - Once it has passed QA and peer review we can merge to develop. Please delete the branch in the github pull request UI when doing this so that we can keep the repository a bit cleaner. - Move the Jira ticket into the resolved status. ## Development practices The JRNI Studio app is several years old and has gone through many previous iterations so there are a lot of old files hanging around with inconsistent styling and different conventions going on. We are striving to continuously improve the quality of the codebase and try to bring things into line with newer conventions. Here are some general good practices that we encourage: ### Opportunistic refactoring Also known as the *good boy scout rule* where you should try to leave things in a better state than when you found them. A good explanation of it is available here: https://martinfowler.com/bliki/OpportunisticRefactoring.html It is better to be inconsistently good than consistently bad. To that end we should try to improve things where we can and not just leave bad practice in place because it is consistent with what is already there. Obviously a certain level of discretion and caution needs to be exercised here. If you have a feeling that a module should be updated but you're not sure just ask another member of the team for some input. This practice has risks which require a good quality set of regression and unit tests to be done safely. ### Testing Write unit tests for your business logic. Keeping business logic out of angular controllers and in plain modules will make this much easier. ## Naming conventions Clarity of purpose is extremely important when naming things. A variable should be descriptive of what it is and a function should tell you what it does with enough context just from the name. It is obviously not always easy to create a concise but descriptive name so there is a bit of give and take in this area but a Pull Request will be a place where this can be discussed and ensure that all parties are happy with the chosen name. Please avoid adding unnecessary comments when a better named function or variable would suffice: **Bad** ```javascript // Called when the booking is checked out function done() {} ``` **Better** ```javascript function onCheckoutComplete(){} ``` Avoid single letter variables except for nameless iterables and when doing array operations as one liners (although it is still preferred to use a full name for clarity) ```javascript for (let i = 0; i < 10; i++) { // do stuff } // OK as the context of the bookings array and the result array explain what is going on const bookingDates = bookings.map(b => b.date) // Preferred const bookingDates = bookings.map(booking => booking.date) // Multi line anonymous functions must use a full name for the variable const bookingDates = bookings.map(booking => { return booking.date; }) ``` ### Variable names Variable names should use `camelCase`. The only exception being that because the API formats everything in `snake_case` it may make sense to use that when building the parameters for a request. ### Method names Methods must be named using `camelCase`. ### File names All file names must be `snake_cased` and saved with `.js` suffix. ### Git branch names All work which has an associated JIRA ticket should exist on a branch with the ticket ID. `SE-1234` for example. This allows JIRA and github integrations to function properly. ## AngularJS Style guide We aim to generally adhere to the following style guide so please familiarise yourself with it: [Angular Style Guide ES2015/ES6](https://github.com/rwwagner90/angular-styleguide-es6) there may be occasional differences in approach due to the nature of the JS ecosystem changing since it was written and the new ES6 features available to us ### AngularJS files Use the following naming scheme to give context to each file's purpose ``` booking_list.service.js booking_list.controller.js booking_list.component.js booking_list.html booking_list.scss ``` ### Prefer $ctrl over specific named controllers AngularJS defaults to using `$ctrl` as the name of a component controller, this makes it much simpler to find in templates when switching between different templates and makes things more concise ### Use ES6 modules instead of AngularJS services We are transitioning away from using AngularJS so we want to put all business logic code in a plain JS module instead of an AngularJS service. Depending on the usage you may wish to export a series of utility functions or a full service singleton. Both are fine. ```javascript // utility functions exported from a JS file import { myFunction } from 'some/file/somewhere.js'; // service import MyService from 'antother/file/service.js'; class MyAngularController { constructor() { myFunction(123); MyService.doSomething(); } } ``` If you need something to behave like a stateful singleton as some AngularJS services are you can simply export a single instance from the ES6 module ```javascript class MyStatefulSingleton {} const mySingleton = new MyStatefulSingleton(); export default mySingleton; ``` ## Making a Pull Request The following guidelines are to make it easy for everyone to review each others code and ensure we are able to get things merged in an efficient manner. ### A Pull Request is the responsibility of the author It is your responsibility as the person opening the pull request to ensure that it has been tested and that all feedback has been responded to in a timely manner. Opening a pull request and expecting it to be dealt with by other people without any further input from yourself will result in it not being dealt with. ### A clear and descriptive title A concise and descriptive title making it easy to see at a glance what it is about The title should include the JIRA ticket if there is one in the following format `[SE-1234] Add booking list component to dashboard` ### Fill out the template in the github editor When opening the github pull request form the box will be auto populated with the contents of the pull request template. It is expected that all the relevant information is provided particularly: * A clear description of what the changes are trying to achieve and how. We can see what the code is doing but that may not necessarily be what it is supposed to be doing. * Context about why it is being done is important as the approach may not necessarily be the best one. * A URL to a development deploy must be present so that the reviewer may test the code without having to check it out themselves, this is usually the same URL as is provided to the QA team while testing the ticket. * Screen shots of any significant UI updates - not always relevant but it is nice to give the reviewer visual context especially when lots of CSS is changing to fix a specific problem Obviously not every PR will require a full list of info but generally enough information should be provided so that a reviewer with no previous context is able to understand the intent of the PR without having to ask too many questions.