Feedback
#Feedback II like the usage of an event delegate in the JavaScript code - it is nice how the click event is bound once. Also, the event handling code is separated from the markup - great job!
#Suggestions
Suggestions
##Javascript
Javascript
Data format
###Data format InsteadInstead of using substrings to separate the operands and operator, String.split() could be used to separate the expression into an array of pieces. Actually, maybe it would be wise to store the expression using that data structure and display it by by calling Array.join() on the array from calling Object.values().
Cache DOM references
###Cache DOM references
ThereThere are multiple places where $('#Expression')
exists - that could be stored in a variable when the DOM is ready (e.g. passed to the Expression constructor function, which would store it on this
) and then that variable referenced instead of fetching it each time.
if
chains (Javascript)
###if
chains (Javascript)
YouYou mentioned (when discussing the PHP code) that you don't like long if
chains. But it appears that there is a decent if
chain in the Javascript code that delegates function calls based on the button that was clicked. You could likely simplify that by creating a mapping of buttonText values to functions of Expression
let
vs const
###let
vs const
InIn the function Expression.prototype.Submit
, there is a variable Data
declared using let
. It appears that value doesn't change within the lifetime of that function. Why not use const
instead? The same applies to buttonText
in the click handler.
###jQuery wrapper on event
jQuery wrapper on event
The line:
let buttonText = $(e).attr('target').innerHTML;
Could be simplified to merely:
const buttonText = e.target.innerHTML;
Because there isn't much need in passing the event object in the jQuery wrapper just to get the innerHTML of the target (and as mentioned above, the value doesn't appear to get re-assigned so const
can be used).
For more context, refer to the article below. I know it comes off in the beginning as tough on jQuery but it has some useful information. Stop writing Slow Javascript.
PHP
Unused variable: $status
##PHP
###Unused variable: $status
ItIt appears that variable $status
is unused. Was that used for debugging, or checking the value of $code
?
Mapping built-in functions
###Mapping built-in functions
TheThe static anonymous that call built-in functions (e.g. min()
, max()
, pow()
) could just be replaced by the name of the function in a string literal.
$OperationMap['^'] = 'pow';
$OperationMap['⋀'] = 'max';
$OperationMap['∨'] = 'min';
#Feedback I like the usage of an event delegate in the JavaScript code - it is nice how the click event is bound once. Also, the event handling code is separated from the markup - great job!
#Suggestions
##Javascript
###Data format Instead of using substrings to separate the operands and operator, String.split() could be used to separate the expression into an array of pieces. Actually, maybe it would be wise to store the expression using that data structure and display it by by calling Array.join() on the array from calling Object.values().
###Cache DOM references
There are multiple places where $('#Expression')
exists - that could be stored in a variable when the DOM is ready (e.g. passed to the Expression constructor function, which would store it on this
) and then that variable referenced instead of fetching it each time.
###if
chains (Javascript)
You mentioned (when discussing the PHP code) that you don't like long if
chains. But it appears that there is a decent if
chain in the Javascript code that delegates function calls based on the button that was clicked. You could likely simplify that by creating a mapping of buttonText values to functions of Expression
###let
vs const
In the function Expression.prototype.Submit
, there is a variable Data
declared using let
. It appears that value doesn't change within the lifetime of that function. Why not use const
instead? The same applies to buttonText
in the click handler.
###jQuery wrapper on event
The line:
let buttonText = $(e).attr('target').innerHTML;
Could be simplified to merely:
const buttonText = e.target.innerHTML;
Because there isn't much need in passing the event object in the jQuery wrapper just to get the innerHTML of the target (and as mentioned above, the value doesn't appear to get re-assigned so const
can be used).
For more context, refer to the article below. I know it comes off in the beginning as tough on jQuery but it has some useful information. Stop writing Slow Javascript.
##PHP
###Unused variable: $status
It appears that variable $status
is unused. Was that used for debugging, or checking the value of $code
?
###Mapping built-in functions
The static anonymous that call built-in functions (e.g. min()
, max()
, pow()
) could just be replaced by the name of the function in a string literal.
$OperationMap['^'] = 'pow';
$OperationMap['⋀'] = 'max';
$OperationMap['∨'] = 'min';
Feedback
I like the usage of an event delegate in the JavaScript code - it is nice how the click event is bound once. Also, the event handling code is separated from the markup - great job!
Suggestions
Javascript
Data format
Instead of using substrings to separate the operands and operator, String.split() could be used to separate the expression into an array of pieces. Actually, maybe it would be wise to store the expression using that data structure and display it by by calling Array.join() on the array from calling Object.values().
Cache DOM references
There are multiple places where $('#Expression')
exists - that could be stored in a variable when the DOM is ready (e.g. passed to the Expression constructor function, which would store it on this
) and then that variable referenced instead of fetching it each time.
if
chains (Javascript)
You mentioned (when discussing the PHP code) that you don't like long if
chains. But it appears that there is a decent if
chain in the Javascript code that delegates function calls based on the button that was clicked. You could likely simplify that by creating a mapping of buttonText values to functions of Expression
let
vs const
In the function Expression.prototype.Submit
, there is a variable Data
declared using let
. It appears that value doesn't change within the lifetime of that function. Why not use const
instead? The same applies to buttonText
in the click handler.
jQuery wrapper on event
The line:
let buttonText = $(e).attr('target').innerHTML;
Could be simplified to merely:
const buttonText = e.target.innerHTML;
Because there isn't much need in passing the event object in the jQuery wrapper just to get the innerHTML of the target (and as mentioned above, the value doesn't appear to get re-assigned so const
can be used).
For more context, refer to the article below. I know it comes off in the beginning as tough on jQuery but it has some useful information. Stop writing Slow Javascript.
PHP
Unused variable: $status
It appears that variable $status
is unused. Was that used for debugging, or checking the value of $code
?
Mapping built-in functions
The static anonymous that call built-in functions (e.g. min()
, max()
, pow()
) could just be replaced by the name of the function in a string literal.
$OperationMap['^'] = 'pow';
$OperationMap['⋀'] = 'max';
$OperationMap['∨'] = 'min';
#Feedback I like the usage of an event delegate in the JavaScript code - it is nice how the click event is bound once. Also, the event handling code is separated from the markup - great job!
#Suggestions
##Javascript
###Data format Instead of using substrings to separate the operands and operator, String.split() could be used to separate the expression into an array of pieces. Actually, maybe it would be wise to store the expression using that data structure and display it by by calling Array.join() on the array from calling Object.values().
###Cache DOM references
There are multiple places where $('#Expression')
exists - that could be stored in a variable when the DOM is ready (e.g. passed to the Expression constructor function, which would store it on this
) and then that variable referenced instead of fetching it each time.
###if
chains (Javascript)
You mentioned (when discussing the PHP code) that you don't like long if
chains. But it appears that there is a decent if
chain in the Javascript code that delegates function calls based on the button that was clicked. You could likely simplify that by creating a mapping of buttonText values to functions of Expression
###let
vs const
In the function Expression.prototype.Submit
, there is a variable Data
declared using let
. It appears that value doesn't change within the lifetime of that function. Why not use const
instead? The same applies to buttonText
in the click handler.
###jQuery wrapper on event
The line:
let buttonText = $(e).attr('target').innerHTML;
Could be simplified to merely:
const buttonText = e.target.innerHTML;
Because there isn't much need in passing the event object in the jQuery wrapper just to get the innerHTML of the target (and as mentioned above, the value doesn't appear to get re-assigned so const
can be used).
For more context, refer to the article below. I know it comes off in the beginning as tough on jQuery but it has some useful information. Stop writing Slow Javascript .
##PHP
###Unused variable: $status
It appears that variable $status
is unused. Was that used for debugging, or checking the value of $code
?
###Mapping built-in functions
The static anonymous that call built-in functions (e.g. min()
, max()
, pow()
) could just be replaced by the name of the function in a string literal.
$OperationMap['^'] = 'pow';
$OperationMap['⋀'] = 'max';
$OperationMap['∨'] = 'min';
#Feedback I like the usage of an event delegate in the JavaScript code - it is nice how the click event is bound once. Also, the event handling code is separated from the markup - great job!
#Suggestions
##Javascript
###Data format Instead of using substrings to separate the operands and operator, String.split() could be used to separate the expression into an array of pieces. Actually, maybe it would be wise to store the expression using that data structure and display it by by calling Array.join() on the array from calling Object.values().
###Cache DOM references
There are multiple places where $('#Expression')
exists - that could be stored in a variable when the DOM is ready (e.g. passed to the Expression constructor function, which would store it on this
) and then that variable referenced instead of fetching it each time.
###if
chains (Javascript)
You mentioned (when discussing the PHP code) that you don't like long if
chains. But it appears that there is a decent if
chain in the Javascript code that delegates function calls based on the button that was clicked. You could likely simplify that by creating a mapping of buttonText values to functions of Expression
###let
vs const
In the function Expression.prototype.Submit
, there is a variable Data
declared using let
. It appears that value doesn't change within the lifetime of that function. Why not use const
instead? The same applies to buttonText
in the click handler.
###jQuery wrapper on event
The line:
let buttonText = $(e).attr('target').innerHTML;
Could be simplified to merely:
const buttonText = e.target.innerHTML;
Because there isn't much need in passing the event object in the jQuery wrapper just to get the innerHTML of the target (and as mentioned above, the value doesn't appear to get re-assigned so const
can be used).
##PHP
###Unused variable: $status
It appears that variable $status
is unused. Was that used for debugging, or checking the value of $code
?
###Mapping built-in functions
The static anonymous that call built-in functions (e.g. min()
, max()
, pow()
) could just be replaced by the name of the function in a string literal.
$OperationMap['^'] = 'pow';
$OperationMap['⋀'] = 'max';
$OperationMap['∨'] = 'min';
#Feedback I like the usage of an event delegate in the JavaScript code - it is nice how the click event is bound once. Also, the event handling code is separated from the markup - great job!
#Suggestions
##Javascript
###Data format Instead of using substrings to separate the operands and operator, String.split() could be used to separate the expression into an array of pieces. Actually, maybe it would be wise to store the expression using that data structure and display it by by calling Array.join() on the array from calling Object.values().
###Cache DOM references
There are multiple places where $('#Expression')
exists - that could be stored in a variable when the DOM is ready (e.g. passed to the Expression constructor function, which would store it on this
) and then that variable referenced instead of fetching it each time.
###if
chains (Javascript)
You mentioned (when discussing the PHP code) that you don't like long if
chains. But it appears that there is a decent if
chain in the Javascript code that delegates function calls based on the button that was clicked. You could likely simplify that by creating a mapping of buttonText values to functions of Expression
###let
vs const
In the function Expression.prototype.Submit
, there is a variable Data
declared using let
. It appears that value doesn't change within the lifetime of that function. Why not use const
instead? The same applies to buttonText
in the click handler.
###jQuery wrapper on event
The line:
let buttonText = $(e).attr('target').innerHTML;
Could be simplified to merely:
const buttonText = e.target.innerHTML;
Because there isn't much need in passing the event object in the jQuery wrapper just to get the innerHTML of the target (and as mentioned above, the value doesn't appear to get re-assigned so const
can be used).
For more context, refer to the article below. I know it comes off in the beginning as tough on jQuery but it has some useful information. Stop writing Slow Javascript .
##PHP
###Unused variable: $status
It appears that variable $status
is unused. Was that used for debugging, or checking the value of $code
?
###Mapping built-in functions
The static anonymous that call built-in functions (e.g. min()
, max()
, pow()
) could just be replaced by the name of the function in a string literal.
$OperationMap['^'] = 'pow';
$OperationMap['⋀'] = 'max';
$OperationMap['∨'] = 'min';
#Feedback I like the usage of an event delegate in the JavaScript code - it is nice how the click event is bound once. Also, the event handling code is separated from the markup - great job!
#Suggestions
##Javascript
###Data format Instead of using substrings to separate the operands and operator, String.split() could be used to separate the expression into an array of pieces. Actually, maybe it would be wise to store the expression using that data structure and display it by by calling Array.join() on the array from calling Object.values().
###Cache DOM references
There are multiple places where $('#Expression')
exists - that could be stored in a variable when the DOM is ready (e.g. passed to the Expression constructor function, which would store it on this
) and then that variable referenced instead of fetching it each time.
###if
chains (Javascript)
You mentioned (when discussing the PHP code) that you don't like long if
chains. But it appears that there is a decent if
chain in the Javascript code that delegates function calls based on the button that was clicked. You could likely simplify that by creating a mapping of buttonText values to functions of Expression
###let
vs const
In the function Expression.prototype.Submit
, there is a variable Data
declared using let
. It appears that value doesn't change within the lifetime of that function. Why not use const
instead? The same applies to buttonText
in the click handler.
###jQuery wrapper on event
The line:
let buttonText = $(e).attr('target').innerHTML;
Could be simplified to merely:
const buttonText = e.target.innerHTML;
Because there isn't much need in passing the event object in the jQuery wrapper just to get the innerHTML of the target (likeand as mentioned above, the value doesn't appear to get re-assigned so const
can be used).
##PHP
###Unused variable: $status
It appears that variable $status
is unused. Was that used for debugging, or checking the value of $code
?
###Mapping built-in functions
The static anonymous that call built-in functions (e.g. min()
, max()
, pow()
) could just be replaced by the name of the function in a string literal.
$OperationMap['^'] = 'pow';
$OperationMap['⋀'] = 'max';
$OperationMap['∨'] = 'min';
#Feedback I like the usage of an event delegate in the JavaScript code - it is nice how the click event is bound once. Also, the event handling code is separated from the markup - great job!
#Suggestions
##Javascript
###Data format Instead of using substrings to separate the operands and operator, String.split() could be used to separate the expression into an array of pieces. Actually, maybe it would be wise to store the expression using that data structure and display it by by calling Array.join() on the array from calling Object.values().
###Cache DOM references
There are multiple places where $('#Expression')
exists - that could be stored in a variable when the DOM is ready (e.g. passed to the Expression constructor function, which would store it on this
) and then that variable referenced instead of fetching it each time.
###if
chains (Javascript)
You mentioned (when discussing the PHP code) that you don't like long if
chains. But it appears that there is a decent if
chain in the Javascript code that delegates function calls based on the button that was clicked. You could likely simplify that by creating a mapping of buttonText values to functions of Expression
###let
vs const
In the function Expression.prototype.Submit
, there is a variable Data
declared using let
. It appears that value doesn't change within the lifetime of that function. Why not use const
instead? The same applies to buttonText
in the click handler.
###jQuery wrapper on event
The line:
let buttonText = $(e).attr('target').innerHTML;
Could be simplified to merely:
const buttonText = e.target.innerHTML;
Because there isn't much need in passing the event object in the jQuery wrapper just to get the innerHTML of the target (like mentioned above, the value doesn't appear to get re-assigned so const
can be used).
##PHP
###Unused variable: $status
It appears that variable $status
is unused. Was that used for debugging, or checking the value of $code
?
###Mapping built-in functions
The static anonymous that call built-in functions (e.g. min()
, max()
, pow()
) could just be replaced by the name of the function in a string literal.
$OperationMap['^'] = 'pow';
$OperationMap['⋀'] = 'max';
$OperationMap['∨'] = 'min';
#Feedback I like the usage of an event delegate in the JavaScript code - it is nice how the click event is bound once. Also, the event handling code is separated from the markup - great job!
#Suggestions
##Javascript
###Data format Instead of using substrings to separate the operands and operator, String.split() could be used to separate the expression into an array of pieces. Actually, maybe it would be wise to store the expression using that data structure and display it by by calling Array.join() on the array from calling Object.values().
###Cache DOM references
There are multiple places where $('#Expression')
exists - that could be stored in a variable when the DOM is ready (e.g. passed to the Expression constructor function, which would store it on this
) and then that variable referenced instead of fetching it each time.
###if
chains (Javascript)
You mentioned (when discussing the PHP code) that you don't like long if
chains. But it appears that there is a decent if
chain in the Javascript code that delegates function calls based on the button that was clicked. You could likely simplify that by creating a mapping of buttonText values to functions of Expression
###let
vs const
In the function Expression.prototype.Submit
, there is a variable Data
declared using let
. It appears that value doesn't change within the lifetime of that function. Why not use const
instead? The same applies to buttonText
in the click handler.
###jQuery wrapper on event
The line:
let buttonText = $(e).attr('target').innerHTML;
Could be simplified to merely:
const buttonText = e.target.innerHTML;
Because there isn't much need in passing the event object in the jQuery wrapper just to get the innerHTML of the target (and as mentioned above, the value doesn't appear to get re-assigned so const
can be used).
##PHP
###Unused variable: $status
It appears that variable $status
is unused. Was that used for debugging, or checking the value of $code
?
###Mapping built-in functions
The static anonymous that call built-in functions (e.g. min()
, max()
, pow()
) could just be replaced by the name of the function in a string literal.
$OperationMap['^'] = 'pow';
$OperationMap['⋀'] = 'max';
$OperationMap['∨'] = 'min';