From acb098c296303f073c8a33f297125ce716b0a1a6 Mon Sep 17 00:00:00 2001 From: Scott Picquerey Date: Thu, 31 Mar 2022 16:29:10 +0200 Subject: [PATCH 1/7] Add best practices to readme --- README.md | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index d6bfecf..ed8c0ed 100644 --- a/README.md +++ b/README.md @@ -7,20 +7,101 @@ The exercices consist of unimplemented functions and their associated failing tests. To run the tests, simply run + ```sh $ yarn test ``` You can also run them in watch mode: + ```sh $ yarn test:watch ``` Finally, if you wish to only run the tests for a given exercice `exoN`, you can run the following: + ```sh $ yarn test[:watch] exoN ``` The exercices are organized into `exoN` folders and most of what is required to -complete each is detailed in the comments. \ No newline at end of file +complete each is detailed in the comments. + +## fp-ts typescript style guide + +- Use `flow` instead of `pipe` when possible + +```typescript +// Bad +const myFunc = (user: User) => pipe(user, getUserName, formatUserName); + +// Good +const myFunc = flow(getUserName, formatUserName); +``` + +- Avoid using `boolean` method `match` when unecessary + > Why? boolean.match can reduce the global understanding of a method and enforce nested pipes. Using classic `if/else` is often the best option + +```typescript +// Bad +const myFunc = (condition: boolean) => pipe( + condition === true, + boolean.match( + () => doSomethingOnFalse(...), + () => doSomethingOnTrue(...) + ) +); + +// Good +const myFunc = (condition: boolean) => { + if (condition === true) { + return doSomethingOnTrue(...); + } + return doSomethingOnFalse(...); +}; +``` + +- Split piping from logical actions + > Why? Mixing them makes it harder to understand and read the code. We want to favor the usage of atomic methods that do not know anything about the piping + +```typescript +// Bad +const myFunc = () => pipe( + rte.Do, + rte.bind('aggregate', () => getAggregateById(...)), + rte.bindW(...), + ..., + rte.chainW(({ aggregate }) => pipe( + aggregate.something, + doSomething, + doSomethingElse, + ... + ) + ), + rte.chainW(storeAggregate), +); + +// Good +const myFunc = () => pipe( + rte.Do, + rte.bind('aggregate', () => getAggregateById(...)), + rte.bindW(...), + ..., + rte.chainW(doEverything), + rte.chainW(storeAggregate), +); + +// Best +const buildProps = (...) => pipe( + rte.Do, + rte.bind('aggregate', () => getAggregateById(...)), + rte.bindW(...), + ... +); +const myFunc = () => pipe( + buildProps(...), + rte.chainW(doEverything), + rte.chainW(storeAggregate), +); +``` From 9bda724a6800e53f2089d67a7495d1d5926dfd36 Mon Sep 17 00:00:00 2001 From: Scott Picquerey Date: Fri, 1 Apr 2022 10:00:29 +0200 Subject: [PATCH 2/7] Update wording --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index ed8c0ed..2a37085 100644 --- a/README.md +++ b/README.md @@ -28,7 +28,7 @@ $ yarn test[:watch] exoN The exercices are organized into `exoN` folders and most of what is required to complete each is detailed in the comments. -## fp-ts typescript style guide +## code style guide - Use `flow` instead of `pipe` when possible @@ -41,7 +41,7 @@ const myFunc = flow(getUserName, formatUserName); ``` - Avoid using `boolean` method `match` when unecessary - > Why? boolean.match can reduce the global understanding of a method and enforce nested pipes. Using classic `if/else` is often the best option + > Why? boolean.match can lower the global understanding of a method and enforce nested pipes. Using classic `if/else` is often the best option ```typescript // Bad From a8c6a4bb7e17ff9b804d4dc4f9dd8883c310f32c Mon Sep 17 00:00:00 2001 From: Scott Picquerey Date: Fri, 1 Apr 2022 11:21:53 +0200 Subject: [PATCH 3/7] Update piping example --- README.md | 108 +++++++++++++++++++++++++++++------------------------- 1 file changed, 59 insertions(+), 49 deletions(-) diff --git a/README.md b/README.md index 2a37085..a6e51c0 100644 --- a/README.md +++ b/README.md @@ -30,14 +30,17 @@ complete each is detailed in the comments. ## code style guide +For readability purpose, we replace `ReaderTaskEither` by `rte` + - Use `flow` instead of `pipe` when possible ```typescript // Bad -const myFunc = (user: User) => pipe(user, getUserName, formatUserName); +const formatUserPhoneNumber = (user: User) => + pipe(user, User.phoneNumber, User.formatPhoneNumber); // Good -const myFunc = flow(getUserName, formatUserName); +const formatUserPhoneNumber = flow(User.phoneNumber, User.formatPhoneNumber); ``` - Avoid using `boolean` method `match` when unecessary @@ -45,63 +48,70 @@ const myFunc = flow(getUserName, formatUserName); ```typescript // Bad -const myFunc = (condition: boolean) => pipe( - condition === true, - boolean.match( - () => doSomethingOnFalse(...), - () => doSomethingOnTrue(...) - ) -); +const triggerEmailCampaign = ({ + user, + ...emailSettings +}: { + user: User & EmailSettings; +}) => + pipe( + user.nationality === 'FR', + boolean.match( + () => triggerGlobalEmailCampain({ to: user.email, emailSettings }), + () => triggerFrenchEmailCampaign({ to: user.email, emailSettings }), + ), + ); // Good -const myFunc = (condition: boolean) => { - if (condition === true) { - return doSomethingOnTrue(...); +const triggerEmailCampaign = ({ + user, + ...otherProps +}: { + user: User & OtherProps; +}) => { + if (user.nationality === 'FR') { + return triggerFrenchEmailCampaign({ to: user.email, emailSettings }); } - return doSomethingOnFalse(...); + return triggerGlobalEmailCampain({ to: user.email, emailSettings }); }; ``` -- Split piping from logical actions - > Why? Mixing them makes it harder to understand and read the code. We want to favor the usage of atomic methods that do not know anything about the piping +- Avoid nested pipes + > Why? They lower global understanding of the code. We allow ourselves 2 levels of piping maximum per function and tend to do atomic functions instead ```typescript // Bad -const myFunc = () => pipe( - rte.Do, - rte.bind('aggregate', () => getAggregateById(...)), - rte.bindW(...), - ..., - rte.chainW(({ aggregate }) => pipe( - aggregate.something, - doSomething, - doSomethingElse, - ... - ) - ), - rte.chainW(storeAggregate), -); +const renewUserToken = (user: User) => + pipe( + user.token, + option.match( + () => AuthClient.createToken(user), + (token) => + flow( + AuthClient.renewToken(user), + rte.chainW(({ newToken }) => + pipe(newToken.hash, user.updateToken, rte.chainEitherKW(storeUser)), + ), + ), + ), + ); // Good -const myFunc = () => pipe( - rte.Do, - rte.bind('aggregate', () => getAggregateById(...)), - rte.bindW(...), - ..., - rte.chainW(doEverything), - rte.chainW(storeAggregate), -); +const updateUserToken = (user: User) => (newToken: Token) => + pipe(newToken.hash, user.updateToken, rte.chainEitherKW(storeUser)); -// Best -const buildProps = (...) => pipe( - rte.Do, - rte.bind('aggregate', () => getAggregateById(...)), - rte.bindW(...), - ... -); -const myFunc = () => pipe( - buildProps(...), - rte.chainW(doEverything), - rte.chainW(storeAggregate), -); +const renewAndUpdateUserToken = (user: User) => + flow( + AuthClient.renewToken(user), + rte.chainW(({ newToken }) => updateUserToken(user)), + ); + +const renewUserToken = (user: User) => + pipe( + user.token, + option.match( + () => AuthClient.createToken(user), + renewAndUpdateUserToken(user), + ), + ); ``` From 6fe9e04f45dd8b711be9b03ba4786ab3e39bac84 Mon Sep 17 00:00:00 2001 From: Scott Picquerey Date: Wed, 6 Apr 2022 14:20:55 +0200 Subject: [PATCH 4/7] PR review fix & improvements --- README.md | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index a6e51c0..6afa661 100644 --- a/README.md +++ b/README.md @@ -33,6 +33,7 @@ complete each is detailed in the comments. For readability purpose, we replace `ReaderTaskEither` by `rte` - Use `flow` instead of `pipe` when possible + > Why? Using flow reduces the amount of variables to declare in a method, hence the visibility and readability of the code ```typescript // Bad @@ -52,8 +53,7 @@ const triggerEmailCampaign = ({ user, ...emailSettings }: { - user: User & EmailSettings; -}) => + user: User} & EmailSettings) => pipe( user.nationality === 'FR', boolean.match( @@ -65,15 +65,12 @@ const triggerEmailCampaign = ({ // Good const triggerEmailCampaign = ({ user, - ...otherProps -}: { - user: User & OtherProps; -}) => { + ...emailSettings +}: { user: User } & EmailSettings) => { if (user.nationality === 'FR') { return triggerFrenchEmailCampaign({ to: user.email, emailSettings }); } return triggerGlobalEmailCampain({ to: user.email, emailSettings }); -}; ``` - Avoid nested pipes @@ -81,14 +78,14 @@ const triggerEmailCampaign = ({ ```typescript // Bad -const renewUserToken = (user: User) => +const refreshUserToken = (user: User) => pipe( user.token, option.match( () => AuthClient.createToken(user), (token) => - flow( - AuthClient.renewToken(user), + pipe( + AuthClient.refreshToken({ user, token }), rte.chainW(({ newToken }) => pipe(newToken.hash, user.updateToken, rte.chainEitherKW(storeUser)), ), @@ -97,16 +94,16 @@ const renewUserToken = (user: User) => ); // Good -const updateUserToken = (user: User) => (newToken: Token) => - pipe(newToken.hash, user.updateToken, rte.chainEitherKW(storeUser)); +const updateUserToken = (user: User) => + flow(user.updateToken, rte.chainEitherKW(storeUser)); -const renewAndUpdateUserToken = (user: User) => - flow( - AuthClient.renewToken(user), - rte.chainW(({ newToken }) => updateUserToken(user)), +const refreshAndUpdateUserToken = (user: User) => (token: Token) => + pipe( + AuthClient.refreshToken({ user, token }), + rte.chainW(({ newToken }) => updateUserToken(user)(newToken.hash)), ); -const renewUserToken = (user: User) => +const refreshUserToken = (user: User) => pipe( user.token, option.match( From e056cdc02c75040d815cf858a33542ec7d42abb4 Mon Sep 17 00:00:00 2001 From: Scott Picquerey Date: Wed, 6 Apr 2022 15:26:42 +0200 Subject: [PATCH 5/7] Update example for nested pipes --- README.md | 65 +++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 46 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 6afa661..e37895b 100644 --- a/README.md +++ b/README.md @@ -78,37 +78,64 @@ const triggerEmailCampaign = ({ ```typescript // Bad -const refreshUserToken = (user: User) => +const convertDollarAmountInCountryCurrency = ({ + countryName, + amountInDollar, +}: { + countryName: CountryName; + amountInDollar: number; +}) => pipe( - user.token, - option.match( - () => AuthClient.createToken(user), - (token) => + getCountryCode(countryName), + either.map( + countryCode => pipe( - AuthClient.refreshToken({ user, token }), - rte.chainW(({ newToken }) => - pipe(newToken.hash, user.updateToken, rte.chainEitherKW(storeUser)), + getCountryCurrency(countryCode), + option.map(currency => + pipe( + amountInDollar, + converFromDollar(currency), + convertedAmount => + console.log( + `converted amount for country ${countryCode} is ${convertedAmount}`, + ), + ), + ), ), ), ), ); // Good -const updateUserToken = (user: User) => - flow(user.updateToken, rte.chainEitherKW(storeUser)); - -const refreshAndUpdateUserToken = (user: User) => (token: Token) => +const convertDollarAmountInCountryCodeCurrency = ({ + amountInDollar, + countryCode, +}: { + amountInDollar: number; + countryCode: CountryCode; +}) => pipe( - AuthClient.refreshToken({ user, token }), - rte.chainW(({ newToken }) => updateUserToken(user)(newToken.hash)), + getCurrencyFromCountryCode(countryCode), + option.map(currency => { + const convertedDollarAmount = convertFromDollar(currency)(currency); + console.log( + `converted amount for country ${countryCode} is ${convertedAmount}`, + ); + }), ); -const refreshUserToken = (user: User) => +const convertDollarAmountInCountryCurrency = ({ + amountInDollar, + countryName, +}: { + amountInDollar: number; + countryName: CountryName; +}) => pipe( - user.token, - option.match( - () => AuthClient.createToken(user), - renewAndUpdateUserToken(user), + getCountryCode(countryName), + either.map(countryCode => + convertDollarAmountToCountryCodeCurrency({ amountInDollar, countryCode }), ), ); + ``` From cb043aec01e5fa5c402655cc50123b392210574a Mon Sep 17 00:00:00 2001 From: Scott Picquerey Date: Wed, 6 Apr 2022 15:26:46 +0200 Subject: [PATCH 6/7] Update prettier rules --- .prettierrc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.prettierrc b/.prettierrc index a20502b..5393aaf 100644 --- a/.prettierrc +++ b/.prettierrc @@ -1,4 +1,5 @@ { "singleQuote": true, - "trailingComma": "all" + "trailingComma": "all", + "arrowParens": "avoid" } From 8ec3fd335ed865ca26bcac80929cfc9c3bc5473f Mon Sep 17 00:00:00 2001 From: Scott Picquerey Date: Wed, 6 Apr 2022 15:52:18 +0200 Subject: [PATCH 7/7] Improve code structure --- README.md | 57 ++++++++++++++++++++----------------------------------- 1 file changed, 21 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index e37895b..0cf4caa 100644 --- a/README.md +++ b/README.md @@ -57,7 +57,7 @@ const triggerEmailCampaign = ({ pipe( user.nationality === 'FR', boolean.match( - () => triggerGlobalEmailCampain({ to: user.email, emailSettings }), + () => triggerGlobalEmailCampaign({ to: user.email, emailSettings }), () => triggerFrenchEmailCampaign({ to: user.email, emailSettings }), ), ); @@ -70,7 +70,7 @@ const triggerEmailCampaign = ({ if (user.nationality === 'FR') { return triggerFrenchEmailCampaign({ to: user.email, emailSettings }); } - return triggerGlobalEmailCampain({ to: user.email, emailSettings }); + return triggerGlobalEmailCampaign({ to: user.email, emailSettings }); ``` - Avoid nested pipes @@ -78,7 +78,7 @@ const triggerEmailCampaign = ({ ```typescript // Bad -const convertDollarAmountInCountryCurrency = ({ +export const convertDollarAmountInCountryCurrency = ({ countryName, amountInDollar, }: { @@ -91,10 +91,9 @@ const convertDollarAmountInCountryCurrency = ({ countryCode => pipe( getCountryCurrency(countryCode), - option.map(currency => - pipe( - amountInDollar, - converFromDollar(currency), + option.map( + flow( + convertFromDollarAmount(amountInDollar), convertedAmount => console.log( `converted amount for country ${countryCode} is ${convertedAmount}`, @@ -107,35 +106,21 @@ const convertDollarAmountInCountryCurrency = ({ ); // Good -const convertDollarAmountInCountryCodeCurrency = ({ - amountInDollar, - countryCode, -}: { - amountInDollar: number; - countryCode: CountryCode; -}) => - pipe( - getCurrencyFromCountryCode(countryCode), - option.map(currency => { - const convertedDollarAmount = convertFromDollar(currency)(currency); - console.log( - `converted amount for country ${countryCode} is ${convertedAmount}`, - ); - }), - ); - -const convertDollarAmountInCountryCurrency = ({ - amountInDollar, - countryName, -}: { - amountInDollar: number; - countryName: CountryName; -}) => - pipe( - getCountryCode(countryName), - either.map(countryCode => - convertDollarAmountToCountryCodeCurrency({ amountInDollar, countryCode }), - ), +export const convertDollarAmountInCountryCurrency = (amountInDollar: number) => + flow( + getCountryCode, + either.map(convertDollarAmountToCountryCodeCurrency(amountInDollar)), ); +const convertDollarAmountInCountryCodeCurrency = + (amountInDollar: number) => (countryCode: CountryCode) => + pipe( + getCurrencyFromCountryCode(countryCode), + option.map(convertFromDollarAmount(amountInDollar)), + option.map(convertedAmount => + console.log( + `converted amount for country ${countryCode} is ${convertedAmount}`, + ), + ), + ); ```