Skip to main content
Code Review

Return to Answer

Bounty Awarded with 25 reputation awarded by Community Bot
added 450 characters in body
Source Link

Questions

Let's look at the second question first:

Or somehow make that first branch clean?

It appears that you already started to analyze that lex function. At 137 lines it is a massive function to read through, let alone try updating. Think of anyone reading the code, including your future self. It would be wise to break that function up into smaller functions. This will also help with testing.

One option would be to use OOP - even if only one instance of an object is needed that object could hold the state in properties instead of referencing variables in a large function. Then that monstrous lex function can be broken up into smaller methods that use properties to update the state.

Is there anything repetitive which could be abstracted out or simplified?

Yes there are quite a few repetitive blocks. For example, the following two blocks appear both within the block starting on line 45 (i.e. if (str.match(/^ *$/)) {) as well as just before the return statement:

 while (nesting > 1) {
 tokens.push({
 form: 'close-paren'
 })
 nesting--
 }
 while (indents.length) {
 tokens.push({
 form: 'close-paren'
 })
 indents.pop()
 }

Those could be re-written as for loops. For example:

 for (; nesting > 1; nesting--) {
 tokens.push({
 form: 'close-paren'
 })
 }
 for (; indents.length; indents.pop()) {
 tokens.push({
 form: 'close-paren'
 })
 }

Additionally, the following three lines appear eight times:

tokens.push({
 form: 'close-paren'
})

Making a separate function/method for that seems excessive (since it would lead to additional function calls just to call the push method on an array). It could be declared once before the function:

const closeParenToken = {
 form: 'close-paren'
};

And then referenced whenever such a token needs to be pushed into the tokens. If having the same object copied each time is an issue then various techniques could be used - e.g. spreading into a new object - {...closeParenToken}

Other review points

Most variables are declared using let. Some could be could be declared with const - e.g. indents, match on line 41, match on line 62, newIndent, oldIndent, etc. It is wise to default to using const, unless you know re-assignment is necessary - then use let. This will help avoid accidental re-assignment and other bugs .

Let's look at the second question first:

Or somehow make that first branch clean?

It appears that you already started to analyze that lex function. At 137 lines it is a massive function to read through, let alone try updating. Think of anyone reading the code, including your future self. It would be wise to break that function up into smaller functions. This will also help with testing.

One option would be to use OOP - even if only one instance of an object is needed that object could hold the state in properties instead of referencing variables in a large function. Then that monstrous lex function can be broken up into smaller methods that use properties to update the state.

Is there anything repetitive which could be abstracted out or simplified?

Yes there are quite a few repetitive blocks. For example, the following two blocks appear both within the block starting on line 45 (i.e. if (str.match(/^ *$/)) {) as well as just before the return statement:

 while (nesting > 1) {
 tokens.push({
 form: 'close-paren'
 })
 nesting--
 }
 while (indents.length) {
 tokens.push({
 form: 'close-paren'
 })
 indents.pop()
 }

Those could be re-written as for loops. For example:

 for (; nesting > 1; nesting--) {
 tokens.push({
 form: 'close-paren'
 })
 }
 for (; indents.length; indents.pop()) {
 tokens.push({
 form: 'close-paren'
 })
 }

Additionally, the following three lines appear eight times:

tokens.push({
 form: 'close-paren'
})

Making a separate function/method for that seems excessive (since it would lead to additional function calls just to call the push method on an array). It could be declared once before the function:

const closeParenToken = {
 form: 'close-paren'
};

And then referenced whenever such a token needs to be pushed into the tokens. If having the same object copied each time is an issue then various techniques could be used - e.g. spreading into a new object - {...closeParenToken}

Questions

Let's look at the second question first:

Or somehow make that first branch clean?

It appears that you already started to analyze that lex function. At 137 lines it is a massive function to read through, let alone try updating. Think of anyone reading the code, including your future self. It would be wise to break that function up into smaller functions. This will also help with testing.

One option would be to use OOP - even if only one instance of an object is needed that object could hold the state in properties instead of referencing variables in a large function. Then that monstrous lex function can be broken up into smaller methods that use properties to update the state.

Is there anything repetitive which could be abstracted out or simplified?

Yes there are quite a few repetitive blocks. For example, the following two blocks appear both within the block starting on line 45 (i.e. if (str.match(/^ *$/)) {) as well as just before the return statement:

 while (nesting > 1) {
 tokens.push({
 form: 'close-paren'
 })
 nesting--
 }
 while (indents.length) {
 tokens.push({
 form: 'close-paren'
 })
 indents.pop()
 }

Those could be re-written as for loops. For example:

 for (; nesting > 1; nesting--) {
 tokens.push({
 form: 'close-paren'
 })
 }
 for (; indents.length; indents.pop()) {
 tokens.push({
 form: 'close-paren'
 })
 }

Additionally, the following three lines appear eight times:

tokens.push({
 form: 'close-paren'
})

Making a separate function/method for that seems excessive (since it would lead to additional function calls just to call the push method on an array). It could be declared once before the function:

const closeParenToken = {
 form: 'close-paren'
};

And then referenced whenever such a token needs to be pushed into the tokens. If having the same object copied each time is an issue then various techniques could be used - e.g. spreading into a new object - {...closeParenToken}

Other review points

Most variables are declared using let. Some could be could be declared with const - e.g. indents, match on line 41, match on line 62, newIndent, oldIndent, etc. It is wise to default to using const, unless you know re-assignment is necessary - then use let. This will help avoid accidental re-assignment and other bugs .

Source Link

Let's look at the second question first:

Or somehow make that first branch clean?

It appears that you already started to analyze that lex function. At 137 lines it is a massive function to read through, let alone try updating. Think of anyone reading the code, including your future self. It would be wise to break that function up into smaller functions. This will also help with testing.

One option would be to use OOP - even if only one instance of an object is needed that object could hold the state in properties instead of referencing variables in a large function. Then that monstrous lex function can be broken up into smaller methods that use properties to update the state.

Is there anything repetitive which could be abstracted out or simplified?

Yes there are quite a few repetitive blocks. For example, the following two blocks appear both within the block starting on line 45 (i.e. if (str.match(/^ *$/)) {) as well as just before the return statement:

 while (nesting > 1) {
 tokens.push({
 form: 'close-paren'
 })
 nesting--
 }
 while (indents.length) {
 tokens.push({
 form: 'close-paren'
 })
 indents.pop()
 }

Those could be re-written as for loops. For example:

 for (; nesting > 1; nesting--) {
 tokens.push({
 form: 'close-paren'
 })
 }
 for (; indents.length; indents.pop()) {
 tokens.push({
 form: 'close-paren'
 })
 }

Additionally, the following three lines appear eight times:

tokens.push({
 form: 'close-paren'
})

Making a separate function/method for that seems excessive (since it would lead to additional function calls just to call the push method on an array). It could be declared once before the function:

const closeParenToken = {
 form: 'close-paren'
};

And then referenced whenever such a token needs to be pushed into the tokens. If having the same object copied each time is an issue then various techniques could be used - e.g. spreading into a new object - {...closeParenToken}

lang-js

AltStyle によって変換されたページ (->オリジナル) /