Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

rather than passing more arguments to the reduce function. The sortRows() function is a nice, generic function (if I do say so myself if I do say so myself) that doesn't care where its input came from. It just sorts it. If you don't want to sort it, well, don't.

rather than passing more arguments to the reduce function. The sortRows() function is a nice, generic function (if I do say so myself) that doesn't care where its input came from. It just sorts it. If you don't want to sort it, well, don't.

rather than passing more arguments to the reduce function. The sortRows() function is a nice, generic function (if I do say so myself) that doesn't care where its input came from. It just sorts it. If you don't want to sort it, well, don't.

deleted 31 characters in body
Source Link
Flambino
  • 33.3k
  • 2
  • 46
  • 90

While you've extracted quite a few functions, they're very tied to an external state. You provide some of the values they need as arguments, but scaleRow (for instance), also relies on pos.pivot. So it's sort of "halfway extracted". it'sIt's not reusable, although the act of scaling a vector is actually a generic operation.

It doesn't have to be, though. What you want is just to multiply every element in an array (aka row/vector) with a value, producing a new, scaled vector. Basically, \$\mathbf{V}\times c\$. You can achieve this like so:

Thisscale() function is completely independent, stateless and reusable. Of course, achieving these qualities isn't necessary for an inlined function (JS has closures for a reason: They're awesome!), but if you're going to extract a function, aim for maximum independence.

function reducedRowEchelonForm(matrix) {
 var knownPivotColumns = []; // this is our one piece of iffy state-keeping :(
 // Copy the input matrix (reusing the variable name) so we have a local copy to work on
 matrix = matrix.map(function (row) { return row.slice() });
 // Now, go through the matrix and do the reduction.
 // We're using forEach here, because we want to update the matrix
 // in-place, whereas `map()` will work on a separate instance
 matrix.forEach(function (row, rowIndex) {
 // Find the row's pivot
 // This is wrapped in an IIFE just for structure
 var pivot = (function () {
 // using a regular for-loop, since we may want to break out of it early
 for(var i = 0, l = row.length ; i < l ; i++ ) {
 if(!row[i] || knownPivotColumns[i]) { continue } // skip column if it's zero or its index is taken
 knownPivotColumns[i] = true; // "reserve" the column
 return { index: i, value: row[i] }; // return the pivot data
 }
 return null; // no pivot found
 }());
 // if there's no pivot, there's nothing else to do for this row
 if(!pivot) { return }
 // scale the row, if necessary
 if(pivot.value !== 1) {
 // using forEach as a "map in place" here
 row.forEach(function (_, i) { row[i] /= pivot.value });
 }
 // now reduce the other rows (calling them "siblings" here)
 matrix.forEach(function (sibling) {
 var siblingPivotValue = sibling[pivot.index];
 if(sibling === row || siblingPivotValue === 0) { return } // skip if possible
 // subtract the sibling-pivot-scaled row from the sibling
 // (another "forEach as map-in-place")
 sibling.forEach(function (_, i) { sibling[i] -= row[i] * siblingPivotValue });
 });
 });
 return matrix;
}

While you've extracted quite a few functions, they're very tied to an external state. You provide some of the values they need as arguments, but scaleRow (for instance), also relies on pos.pivot. So it's sort of "halfway extracted". it's not reusable, although the act of scaling a vector is a generic operation.

It doesn't have to be, though. What you want is just to multiply every element in an array (aka row/vector) with a value, producing a new, scaled vector. Basically, \$\mathbf{V}\times c\$. You can achieve this like so:

Thisscale() function is completely independent, stateless and reusable. Of course, achieving these qualities isn't necessary for an inlined function (JS has closures for a reason: They're awesome!), but if you're going to extract a function, aim for maximum independence.

function reducedRowEchelonForm(matrix) {
 var knownPivotColumns = []; // this is our one piece of iffy state-keeping :(
 // Copy the input matrix (reusing the variable name) so we have a local copy to work on
 matrix = matrix.map(function (row) { return row.slice() });
 // Now, go through the matrix and do the reduction.
 // We're using forEach here, because we want to update the matrix
 // in-place, whereas `map()` will work on a separate instance
 matrix.forEach(function (row, rowIndex) {
 // Find the row's pivot
 // This is wrapped in an IIFE just for structure
 var pivot = (function () {
 // using a regular for-loop, since we may want to break out of it early
 for(var i = 0, l = row.length ; i < l ; i++ ) {
 if(!row[i] || knownPivotColumns[i]) { continue } // skip column if it's zero or its index is taken
 knownPivotColumns[i] = true; // "reserve" the column
 return { index: i, value: row[i] }; // return the pivot data
 }
 return null; // no pivot found
 }());
 // if there's no pivot, there's nothing else to do for this row
 if(!pivot) { return }
 // scale the row, if necessary
 if(pivot.value !== 1) {
 // using forEach as a "map in place" here
 row.forEach(function (_, i) { row[i] /= pivot.value });
 }
 // now reduce the other rows (calling them "siblings" here)
 matrix.forEach(function (sibling) {
 var siblingPivotValue = sibling[pivot.index];
 if(sibling === row || siblingPivotValue === 0) { return } // skip if possible
 // subtract the sibling-pivot-scaled row from the sibling
 // (another "forEach as map-in-place")
 sibling.forEach(function (_, i) { sibling[i] -= row[i] * siblingPivotValue });
 });
 });
 return matrix;
}

While you've extracted quite a few functions, they're very tied to an external state. You provide some of the values they need as arguments, but scaleRow (for instance), also relies on pos.pivot. So it's sort of "halfway extracted". It's not reusable, although the act of scaling a vector is actually a generic operation.

What you want is just to multiply every element in an array (aka row/vector) with a value, producing a new, scaled vector. Basically, \$\mathbf{V}\times c\$. You can achieve this like so:

This function is completely independent, stateless and reusable. Of course, achieving these qualities isn't necessary for an inlined function (JS has closures for a reason: They're awesome!), but if you're going to extract a function, aim for maximum independence.

function reducedRowEchelonForm(matrix) {
 var knownPivotColumns = []; // this is our one piece of iffy state-keeping :(
 // Copy the input matrix (reusing the variable name) so we have a local copy to work on
 matrix = matrix.map(function (row) { return row.slice() });
 // Now, go through the matrix and do the reduction.
 // We're using forEach here, because we want to update the matrix
 // in-place, whereas `map()` will work on a separate instance
 matrix.forEach(function (row, rowIndex) {
 // Find the row's pivot
 // This is wrapped in an IIFE just for structure
 var pivot = (function () {
 // using a regular for-loop, since we may want to break out of it early
 for(var i = 0, l = row.length ; i < l ; i++ ) {
 if(!row[i] || knownPivotColumns[i]) { continue } // skip column if it's zero or its index is taken
 knownPivotColumns[i] = true; // "reserve" the column
 return { index: i, value: row[i] }; // return the pivot data
 }
 return null; // no pivot found
 }());
 // if there's no pivot, there's nothing else to do for this row
 if(!pivot) { return }
 // scale the row, if necessary
 if(pivot.value !== 1) {
 // using forEach as a "map in place" here
 row.forEach(function (_, i) { row[i] /= pivot.value });
 }
 // now reduce the other rows (calling them "siblings" here)
 matrix.forEach(function (sibling) {
 var siblingPivotValue = sibling[pivot.index];
 if(sibling === row || siblingPivotValue === 0) { return } // skip if possible
 // subtract the sibling-pivot-scaled row from the sibling
 // (another "forEach as map-in-place")
 sibling.forEach(function (_, i) { sibling[i] -= row[i] * siblingPivotValue });
 });
 });
 return matrix;
}
fixed bug
Source Link
Flambino
  • 33.3k
  • 2
  • 46
  • 90
function reducedRowEchelonForm(matrix) {
 var knownPivotColumns = []; // this is our one piece of iffy state-keeping :(
 // Copy the input matrix (reusing the variable name) so we have a local copy to work on
 matrix = matrix.map(function (row) { return row.slice() });
 // Now, go through the matrix and do the reduction.
 // We're using forEach here, because we want to update the matrix
 // in-place, whereas `map()` will work on a separate instance
 matrix.forEach(function (row, rowIndex) {
 // Find the row's pivot
 // This is wrapped in an IIFE just for structure
 var pivot = (function () {
 // using a regular for-loop, since we may want to break out of it early
 for(var i = 0, l = row.length ; i < l ; i++ ) {
 if(!row[i] || knownPivotColumns[i]) { continue } // skip column if it's zero or its index is taken
 knownPivotColumns[i] = true; // "reserve" the column
 return { columnindex: i, value: row[i] }; // return the pivot data
 }
 return null; // no pivot found
 }());
 // if there's no pivot, there's nothing else to do for this row
 if(!pivot) { return }
 // scale the row, if necessary
 if(pivot.value !== 1) {
 // using forEach as a "map in place" here
 row.forEach(function (_, i) { row[i] /= pivot.value });
 }
 // now reduce the other rows (calling them "siblings" here)
 matrix.forEach(function (sibling) {
 var siblingPivotValue = sibling[pivot.index];
 if(sibling === row || siblingPivotValue === 0) { return } // skip if possible
 // subtract the sibling-pivot-scaled row from the sibling
 // (another "forEach as map-in-place")
 sibling.forEach(function (_, i) { sibling[i] -= row[i] * siblingPivotValue });
 });
 });
 return matrix;
}
function reducedRowEchelonForm(matrix) {
 var knownPivotColumns = []; // this is our one piece of iffy state-keeping :(
 // Copy the input matrix (reusing the variable name) so we have a local copy to work on
 matrix = matrix.map(function (row) { return row.slice() });
 // Now, go through the matrix and do the reduction.
 // We're using forEach here, because we want to update the matrix
 // in-place, whereas `map()` will work on a separate instance
 matrix.forEach(function (row, rowIndex) {
 // Find the row's pivot
 // This is wrapped in an IIFE just for structure
 var pivot = (function () {
 // using a regular for-loop, since we may want to break out of it early
 for(var i = 0, l = row.length ; i < l ; i++ ) {
 if(!row[i] || knownPivotColumns[i]) { continue } // skip column if it's zero or its index is taken
 knownPivotColumns[i] = true; // "reserve" the column
 return { column: i, value: row[i] }; // return the pivot data
 }
 return null; // no pivot found
 }());
 // if there's no pivot, there's nothing else to do for this row
 if(!pivot) { return }
 // scale the row, if necessary
 if(pivot.value !== 1) {
 // using forEach as a "map in place" here
 row.forEach(function (_, i) { row[i] /= pivot.value });
 }
 // now reduce the other rows (calling them "siblings" here)
 matrix.forEach(function (sibling) {
 var siblingPivotValue = sibling[pivot.index];
 if(sibling === row || siblingPivotValue === 0) { return } // skip if possible
 // subtract the sibling-pivot-scaled row from the sibling
 // (another "forEach as map-in-place")
 sibling.forEach(function (_, i) { sibling[i] -= row[i] * siblingPivotValue });
 });
 });
 return matrix;
}
function reducedRowEchelonForm(matrix) {
 var knownPivotColumns = []; // this is our one piece of iffy state-keeping :(
 // Copy the input matrix (reusing the variable name) so we have a local copy to work on
 matrix = matrix.map(function (row) { return row.slice() });
 // Now, go through the matrix and do the reduction.
 // We're using forEach here, because we want to update the matrix
 // in-place, whereas `map()` will work on a separate instance
 matrix.forEach(function (row, rowIndex) {
 // Find the row's pivot
 // This is wrapped in an IIFE just for structure
 var pivot = (function () {
 // using a regular for-loop, since we may want to break out of it early
 for(var i = 0, l = row.length ; i < l ; i++ ) {
 if(!row[i] || knownPivotColumns[i]) { continue } // skip column if it's zero or its index is taken
 knownPivotColumns[i] = true; // "reserve" the column
 return { index: i, value: row[i] }; // return the pivot data
 }
 return null; // no pivot found
 }());
 // if there's no pivot, there's nothing else to do for this row
 if(!pivot) { return }
 // scale the row, if necessary
 if(pivot.value !== 1) {
 // using forEach as a "map in place" here
 row.forEach(function (_, i) { row[i] /= pivot.value });
 }
 // now reduce the other rows (calling them "siblings" here)
 matrix.forEach(function (sibling) {
 var siblingPivotValue = sibling[pivot.index];
 if(sibling === row || siblingPivotValue === 0) { return } // skip if possible
 // subtract the sibling-pivot-scaled row from the sibling
 // (another "forEach as map-in-place")
 sibling.forEach(function (_, i) { sibling[i] -= row[i] * siblingPivotValue });
 });
 });
 return matrix;
}
added 126 characters in body
Source Link
Flambino
  • 33.3k
  • 2
  • 46
  • 90
Loading
added 399 characters in body
Source Link
Flambino
  • 33.3k
  • 2
  • 46
  • 90
Loading
added 341 characters in body
Source Link
Flambino
  • 33.3k
  • 2
  • 46
  • 90
Loading
Source Link
Flambino
  • 33.3k
  • 2
  • 46
  • 90
Loading
default

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