-
Notifications
You must be signed in to change notification settings - Fork 46
Development Best Practices
Sofie Duntugan edited this page Sep 8, 2023
·
3 revisions
- Use descriptive and meaningful names for variables, functions, classes, and other identifiers.
// confusing
const str = 'Hi!';
const fac = 0.10;
// unclear purpose (unless these are established relevant constants app-wide)
const hi = 'Hi!'
const oneTenth = 0.10;
// readable + maintainable
const headerGreeting = 'Hi!';
const toDecimalFactor = 0.10;- Use:
-
camelCasefor variable and function names -
PascalCasefor class, component, interface, and enum names -
UPPER_CASE_SNAKE_CASEfor global constants and enum values - Singular nouns for enums
- Plural nouns for dicts
-
enum DogBreed {
GOLDEN_RETRIEVER ='Golden Retriever',
BOXER = 'Boxer',
HUSKY = 'Husky'
};
const BASE_GROOMING_PRICES = {
[DogBreed.GOLDEN_RETRIEVER]: 20,
[DogBreed.BOXER]: 10,
[DogBreed.HUSKY]: 30
};
const STATE_TAX_RATE = 1.2;
interface Dog {
readonly name: string,
readonly breed: DogBreed,
readonly color: string
};
interface ReceiptProps {
readonly customerName: string;
readonly dogs: Dog[];
};
const Receipt = ({customerName, dogs}: ReceiptProps): JSX.Element => {
const printGroomingPrice = (dogName: string, dogBreed: DogBreed): string => {
const price = BASE_GROOMING_PRICES[dogBreed] * STATE_TAX_RATE;
return `${dogName}: ${price}`;
};
return (
//...
);
}- Prefer string interpolation using template literals over string concatenation with the
+operator.
// lengthy
const amountPaid = itemPrice * taxRate + tip;
const receiptString = customerName + ' paid $' + amountPaid + ' for ' + itemName + '.';
// easier
const receiptString = `${customerName} paid $${itemPrice * taxRate + tip} for ${itemName}.`;- Use object destructuring to extract specific properties from objects in a concise and readable manner.
// messier
const printName = (person: Person) => {
return `Hi ${person.firstName} ${person.lastName}!`;
}
// cleaner
const printName = ({firstName, lastName}: Person) => {
return `Hi ${firstName} ${lastName}!`;
}- Use type annotations when declaring variables, function parameters, and function return types to provide clarity and improve code maintainability.
- Create interfaces when built-in types are insufficient (ex. for objects, including component props).
- Extend types when possible.
interface PaymentInfoProps {
readonly cardType: string,
readonly lastFourCardDigits: number,
readonly billingAddress: string,
readonly billingState: string,
readonly billingZip: string
};
// requires updating both if PaymentInfoProps changes
interface ReceiptProps extends PaymentInfoProps {
readonly customerName: string;
readonly dogs: Dog[];
readonly cardType: string,
readonly lastFourCardDigits: number,
readonly billingAddress: string,
readonly billingState: string,
readonly billingZip: string
};
// more readable and maintainable
interface ReceiptProps extends PaymentInfoProps {
readonly customerName: string;
readonly dogs: Dog[];
};- Avoid using the any type unless absolutely necessary, as it bypasses type checking and may lead to runtime errors.
- Use the pattern
[x, setX]foruseState
// confusing in later use - are the two related?
const [userName, newName] = useState('');
// readable
const [userName, setUserName] = useState('');- Destructure component props for readability unless you only need to pass them in bulk to another component.
// PaymentInfo.tsx
interface PaymentInfoProps {
readonly cardType: string,
readonly lastFourCardDigits: number,
readonly billingAddress: string,
readonly billingState: string,
readonly billingZip: string
};
const PaymentInfo = ({cardType, lastFourCardDigits, billingAddress, billingState, billingZip}: PaymentInfoProps): JSX.Element => {
return (
//...
);
};
// Receipt.tsx
interface ReceiptProps extends PaymentInfoProps {
readonly customerName: string;
readonly dogs: Dog[];
};
const Receipt = ({customerName, dogs, ...props}: ReceiptProps): JSX.Element => {
return (
<>
<p>{customerName}</p>
{dogs.map(
//...
)}
<PaymentInfo {...props} />
</>
);
}- Maintain two folders under src: components and pages.
- components contains components that can be reused across the application, with one folder per component
- pages contains components that make up pages within the application, with one folder per page
- Name folders within components and pages in camelCase after the component it contains. Name the file containing the main component ‘index.tsx’. This enables a clean import and makes it easy for developers to find the component they’re looking for:
import Receipt from 'components/receipt';
// rather than something like
import { Receipt } from 'components/receiptDisplay/mainComponent.tsx';- Break discrete logical or presentational operations into sub-components (think helper functions).
- Have one component per file to maintain readability - even though it may seem unnecessary to isolate a relatively small component related to a single page, the page’s file can become incredibly difficult to read over time and invites merge conflicts with other dev's PRs.
- Place any types or constants used across components in a ‘types.ts’ and/or ‘constants.ts’ file under src.
The first several times I tried to review someone else’s PR, I didn’t actually end up submitting anything. How was I supposed to improve the code of a much smarter developer? I couldn’t find anything wrong with it, but I wasn’t sure I wasn’t missing something. However, I think the most helpful thing is to realize that code reviews in C4C aren’t really about improving code - they’re about making sure that everyone understands parts of the codebase they don’t usually/may soon have to touch and sharing neat tricks and best practices. You don’t have to find something to fix in someone’s code, you just have to ask questions, raise potential flags, celebrate best practices, and/or share your knowledge.
To that end, you should review all of your team's PRs and aim to review some PRs from other teams as well!
This guide is not a checklist of common code mistakes to look out for/best practices to enforce because those things tend to arise naturally as part of this learning and knowledge-sharing process. You should aim to understand the problem and code, then be able to point out how better names would be helpful for future developers/how an alternative solution might be better, rather than to scan the code for missed opportunities to memoize.
Your Tools: Emojis (credit @erikthedeveloper for inspiration)
| Emojis | Meaning |
|---|---|
| 👍 💯 🔥 😍 etc. | “Neat! Love this! Wow! Amazing!” Use this to call out cool pieces of code/things that taught you something wherever possible! |
| ❓ | “Question: Why/What/etc...” Use ❓liberally! Can’t understand what a few lines of code do? Aren’t sure why they have to transform the data? Don’t understand how this fits into the larger product? Asking questions in PRs is the best way to make sure design decisions aren’t siloed away in the author’s head. They can also help the author consider alternative decisions they missed or point out where comments would be helpful. |
| 🤔 💭 | “Hmm. Maybe we can do…/This might…” Use this to think out loud, noting things that might be a source of concern, an alternative solution, or any thoughts you have that could lead to further discussion. |
| ⛏️ | Minor issues that don’t break the code but should be fixed such as styling convention, naming alternatives, and visual UI design decisions. |
| 🔧 | A change that should likely be made. Ex. an edge case that wasn’t addressed. |
| 🏕️ | Clean up the camp! Something that wasn’t part of the author’s PR but should still be cleaned while we have the chance. |
| 📌 | “This should be addressed in a future PR, but this PR made me realize we should acknowledge it…” Leave push pins for out-of-scope issues to document them and make sure everyone has context for where the concern originated/how it might affect this PR. |
| 📝 | Have some relevant knowledge that may or may not require a code change? Leave a note so that everyone has a chance to learn! |
You don’t have to use emojis for all of your comments, but try to use them when possible to help organize your feedback!
Great, so you have tools, but what do you actually do?
- Read the PR description. Understand the problem the developer is trying to solve/the feature they’re implementing. It’s pretty hard to understand the code without this context and this will help you consider edge cases the author might’ve missed. If you need more context, leave a comment with your questions! This’ll help future reviewers and developers looking back on the code as well.
- Pull the author’s code, run it locally, and test it. If it’s an endpoint, call it with an API platform like Postman or Insomnia. If it’s a UI, pull it up and interact with it. The author should’ve left testing instructions, but try to come up with your own tests. Put yourself in the user’s shoes. What happens if you put a null or nonsensical input? Is the UI still reasonable if you shrink the screen? Does the input/output structure make sense to you? Is the UI easy to use? This testing process may reveal some potential changes – ex. “💭It might make more sense if this returns an array instead because…”, “🔧The text is hard to read on mobile (see screenshot below)”, “⛏️Can you rename this input to dateTime instead of date? I think it’d be more indicative of what inputs are valid.” – or it might be a great time to applaud the author “😍This UI is gorgeous!”
- Go through the code. If you found a problem in the testing stage, you can try to recommend a solution here to help save time. Otherwise, just try to understand what the code is doing and consider alternative implementations now that you understand the behavior. The author’s goal is to write code that can be understood by someone who’s never seen it (especially in C4C, where we get new devs all the time!). If you’re struggling, leave questions or suggest that they add comments. If you had to change this code in the future, would it be easy to do it? Would abstractions/alternative organization help? Also consider what knowledge you might be able to share. Have you had to deal with this same problem before and have a great utility function somewhere? If you don’t, would it be a good idea to make that utility function?
When submitting PR Reviews, there are three options:
- Approve - Once you feel like you fully understand the code and don’t have any concerns, approve of it! This might seem like a big deal, but just think of it as a signal that you’ve completed the process. If someone else comes by later and requests changes, no big deal! Understand where they’re coming from, contribute to the new conversation (even just a simple 👍) and approve again when it’s resolved.
- Comment - If you still have outstanding questions or discussions you want to have.
- Request Changes - This blocks someone from merging until you approve, which may feel like a severe option, but don’t be afraid to use it if there’s anything you think should be changed or discussed further. Just be quick to reply to the author’s follow-ups! They can't merge until you resolve this request.
- It’s totally fine to raise concerns that end up being fine! It’s great to walk through several possible alternatives so that we thoroughly understand why the final decision best suited the situation.
- Make sure the language you use adheres to the spirit of PR reviews. Rather than telling someone to make a change, suggest an alternative and explain why you think it might be better (unless it’s a simple style fix, then you can just make the edit as a code suggestion).
- Leave code snippets wherever possible to make it clear what your suggestion would entail! This saves the author from the potentially time-consuming process of trying to figure out what you mean.
- Once the author has resolved your concerns (either by making a change or after a design discussion), resolve your comment in GitHub!