clean code #2289

gabrielsimongianotti · 2021-10-13T16:59:01Z

Description

I made some changes to the code to make it cleaner and more readable

jimmywarting · 2021-10-14T07:15:20Z

src/execute/index.js

@@ -1,5 +1,4 @@
-import getIn from 'lodash/get';
-import isPlainObject from 'lodash/isPlainObject';
+import { isPlainObject, get } from 'lodash';


Hmm, this is more of a pre build friendlier tree shaking thing.
using import isPlainObject from 'lodash/isPlainObject' is more browser friendly as a build-less solution

don't know which one i like the most. :P

ofc i would like to remove lodash entirely

I'll go back as it was before

but do you want to switch lodash to some other npm package?
tell me how it can get better, that I can make the changes

there is an open issue to try and remove it #2187 and replace it with native solutions

like using typeof and optional chaining for get - as long as it's supported by the version swagger do support

Can I make these changes in another PR?

Just for reference: doing this is OK. During building the release fragments we use babel-plugin-lodash and lodash-webpack-plugin.

jimmywarting

lgtm

src/helpers.js

char0n · 2021-10-15T13:57:15Z

Hi @gabrielsimongianotti,

First of all thanks for the work!

Regarding renaming e -> error

This requires lot of changed files and IMHO doesn't provide much of a value. There are multiple conventions how to name the catched error. These includes: e (MDN), ex, error, err (typescript). How would renaming e to error makes code more readable? Both conventions are well known and accepted by the developer community. Choosing one convention and stick with (which brings consistency) is more important to this project. Do you have technical arguments for your change?

jimmywarting · 2021-10-15T14:03:27Z

☝️ agree with you there, i hate linters that require one curtain way of doing things, i just want a linter to warn about errors and not change code style - it's so annoying. one thing doe... i don't like 1 letter variable, annoying do search & replace....

char0n · 2021-10-15T14:28:29Z

one thing doe... i don't like 1 letter variable, annoying do search & replace....

IMHO e in catch statement is one of the few exception I can think of where single letter variable is acceptable for me. Others include counters for loops named i or j or whatever. Personally in JavaScript projects I either use e or error and in TypeScript err.

gabrielsimongianotti · 2021-10-15T17:01:20Z

I changed because in the clean code book, it says that it is bad practice to use letters as variables, I'm going back as it was

char0n · 2021-10-15T18:23:41Z

I changed because in the clean code book, it says that it is bad practice to use letters as variables, I'm going back as it was

Right. Clean code is a great book and I totally agree with what it says. But as mentioned above the catch (e) might just be the exception to the rule, as it's already well established convention in JavaScript community.

char0n · 2021-10-19T13:38:03Z

src/execute/oas3/style-serializer.js

@@ -1,3 +1,5 @@
+import { Buffer } from 'buffer';


Why we added import to Buffer here? We got rid of it in e96702f

char0n · 2021-10-19T13:40:58Z

src/helpers.js

@@ -160,10 +160,10 @@ export function normalizeSwagger(parsedSpec) {

        const opList = map[oid];
        if (opList.length > 1) {
-          opList.forEach((o, i) => {
+          opList.forEach((op, key) => {


Why key here and not index? key is usually used in object context, not in array context

char0n · 2021-10-19T13:47:20Z

src/http/index.js

@@ -202,7 +202,7 @@ function formatKeyValue(key, input, skipEncoding = false) {
  const { collectionFormat, allowEmptyValue, serializationOption, encoding } = input;
  // `input` can be string
  const value = typeof input === 'object' && !Array.isArray(input) ? input.value : input;
-  const encodeFn = skipEncoding ? (k) => k.toString() : (k) => encodeURIComponent(k);
+  const encodeFn = (k) => (skipEncoding ? k.toString() : encodeURIComponent(k));


I'm wondering here what is the motivation and arguments behind your change? If I would to simplify this I'd probably go with the following:

Suggested change

const encodeFn = (k) => (skipEncoding ? k.toString() : encodeURIComponent(k));

const encodeFn = skipEncoding ? String : encodeURIComponent;

Explanation: it's a simple ternary that chooses an appropriate function to be used as value of encodeFn

char0n · 2021-10-19T13:54:41Z

src/http/index.js

@@ -290,8 +290,10 @@ function formatKeyValueBySerializationOption(key, value, skipEncoding, serializa
    : serializationOption && serializationOption.allowReserved
    ? 'unsafe'
    : 'reserved';
-  const encodeFn = (v) => encodeDisallowedCharacters(v, { escape });
-  const encodeKeyFn = skipEncoding ? (k) => k : (k) => encodeDisallowedCharacters(k, { escape });
+  const encodeFn = (str) => encodeDisallowedCharacters(str, { escape });


Using str as variable name is consistent with https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent, so let's go for it.

If our goal is simplifying the cleaning the code what would you say to following?

Suggested change

const encodeFn = (str) => encodeDisallowedCharacters(str, { escape });

const identity = (v) => v;

const encodeFn = (str) => encodeDisallowedCharacters(str, { escape });

const encodeKeyFn = skipEncoding ? identity : encodeFn;

char0n · 2021-10-19T14:00:47Z

src/http/index.js

@@ -432,8 +434,8 @@ export function mergeInQueryOrForm(req = {}) {

 // Wrap a http function ( there are otherways to do this, consider this deprecated )
 export function makeHttp(httpFn, preFetch, postFetch) {
-  postFetch = postFetch || ((a) => a);
-  preFetch = preFetch || ((a) => a);
+  postFetch = postFetch || ((req) => req);


Regarding variable naming - yeah, great change!

(req) => req is an identity function. We already needed one in https://github.com/swagger-api/swagger-js/pull/2289/files#r731893085. Let's define an identity function in top of the file and just use it in my previous comment suggestion and in here.

char0n · 2021-10-19T14:01:40Z

src/specmap/index.js

@@ -97,7 +97,7 @@ class SpecMap {
          return true;
        }

-        return path.every((val, i) => val === tested[i]);
+        return path.every((val, key) => val === tested[key]);


key is associated with objects. Arrays use index naming.

char0n · 2021-10-19T14:03:57Z

src/specmap/index.js

@@ -342,7 +342,7 @@ class SpecMap {

    // A different plugin runs, wait for all promises to resolve, then retry
    if (plugin !== this.currentPlugin && this.promisedPatches.length) {
-      const promises = this.promisedPatches.map((p) => p.value);
+      const promises = this.promisedPatches.map((parameter) => parameter.value);


origina p represent a patch here and not parameter, or am I wrong?

Suggested change

const promises = this.promisedPatches.map((parameter) => parameter.value);

const promises = this.promisedPatches.map((patch) => patch.value);

char0n · 2021-10-19T14:08:41Z

src/specmap/lib/properties.js

@@ -6,9 +6,9 @@ export default {
    const val = { ...properties };

    // eslint-disable-next-line no-restricted-syntax, guard-for-in
-    for (const k in properties) {
+    for (const position in properties) {


How about changing k to key here? It actually says what the variable is in context of for in on object.

char0n · 2021-11-02T08:15:10Z

@gabrielsimongianotti why closing it? We had couple of things to discuss here, and the codebase would benefit from this.

gabrielsimongianotti added 3 commits October 12, 2021 19:38

refactor: simple alteration in imports

e35c4a1

refactor: change the variable names to make the code cleaner

f986710

Merge branch 'master' into hacktoberfest

4471555

jimmywarting reviewed Oct 14, 2021

View reviewed changes

jimmywarting approved these changes Oct 14, 2021

View reviewed changes

refactor: import more browser friendly

1521f82

char0n reviewed Oct 15, 2021

View reviewed changes

src/helpers.js Outdated Show resolved Hide resolved

refactor: going back 'error' to 'e'

42a7f8b

gabrielsimongianotti requested a review from char0n October 16, 2021 18:36

Merge branch 'master' into hacktoberfest

c7f5035

char0n reviewed Oct 19, 2021

View reviewed changes

gabrielsimongianotti closed this Nov 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean code #2289

clean code #2289

gabrielsimongianotti commented Oct 13, 2021

jimmywarting Oct 14, 2021 •

edited

Loading

gabrielsimongianotti Oct 14, 2021

gabrielsimongianotti Oct 14, 2021

jimmywarting Oct 14, 2021

gabrielsimongianotti Oct 15, 2021

jimmywarting Oct 15, 2021

char0n Oct 15, 2021

jimmywarting left a comment

char0n commented Oct 15, 2021

jimmywarting commented Oct 15, 2021 •

edited

Loading

char0n commented Oct 15, 2021

gabrielsimongianotti commented Oct 15, 2021

char0n commented Oct 15, 2021

char0n Oct 19, 2021

char0n Oct 19, 2021

char0n Oct 19, 2021 •

edited

Loading

char0n Oct 19, 2021 •

edited

Loading

char0n Oct 19, 2021

char0n Oct 19, 2021

char0n Oct 19, 2021

char0n Oct 19, 2021

char0n commented Nov 2, 2021

	const encodeFn = (k) => (skipEncoding ? k.toString() : encodeURIComponent(k));
	const encodeFn = skipEncoding ? String : encodeURIComponent;

	const promises = this.promisedPatches.map((parameter) => parameter.value);
	const promises = this.promisedPatches.map((patch) => patch.value);

clean code #2289

clean code #2289

Conversation

gabrielsimongianotti commented Oct 13, 2021

Description

jimmywarting Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmywarting left a comment

Choose a reason for hiding this comment

char0n commented Oct 15, 2021

jimmywarting commented Oct 15, 2021 • edited Loading

char0n commented Oct 15, 2021

gabrielsimongianotti commented Oct 15, 2021

char0n commented Oct 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

char0n Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

char0n Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

char0n commented Nov 2, 2021

jimmywarting Oct 14, 2021 •

edited

Loading

jimmywarting commented Oct 15, 2021 •

edited

Loading

char0n Oct 19, 2021 •

edited

Loading

char0n Oct 19, 2021 •

edited

Loading