Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/

Source Link

edited May 23, 2017 at 12:40

Community Bot

edited May 23, 2017 at 12:40

Community Bot

While the nested ternary seems brilliant, it reveals a violation of DRY ( you could do this with one map because every negative n has a positive equivalent
An Array function should either change the current array or make a new array, your function could do either depending on whether a shift is necessary or not
Rotation really is achieved by taking a part of the string and putting it on the other end, since JavaScripts provides splice and shift I would go with something like this this
Minor, but your indentation is off in this function and elsewhere, it disturbs the reading flow

While the nested ternary seems brilliant, it reveals a violation of DRY ( you could do this with one map because every negative n has a positive equivalent
An Array function should either change the current array or make a new array, your function could do either depending on whether a shift is necessary or not
Rotation really is achieved by taking a part of the string and putting it on the other end, since JavaScripts provides splice and shift I would go with something like this
Minor, but your indentation is off in this function and elsewhere, it disturbs the reading flow

While the nested ternary seems brilliant, it reveals a violation of DRY ( you could do this with one map because every negative n has a positive equivalent
An Array function should either change the current array or make a new array, your function could do either depending on whether a shift is necessary or not
Rotation really is achieved by taking a part of the string and putting it on the other end, since JavaScripts provides splice and shift I would go with something like this
Minor, but your indentation is off in this function and elsewhere, it disturbs the reading flow

Not too shabby attempt at a rewrite :P

Source Link

edited Jul 1, 2016 at 21:41

konijn

edited Jul 1, 2016 at 21:41

konijn

34.3k
5
70
267

I am going to attempt to writewrote an alternative, dont holdit works slightly different. (I like that your breath ;code recognizes the word fox, whereas my code goes too far in finding commonalities) I think for an extra bonus, the code should go for both the largest match and the largest mismatch, whatever is largest should go forward. My code has some idiosyncracies, feel free to adopt or ignore (like using ~ with indexOf or not always using curly braces with if statements.)

//The idea is that we try to match the original string, 
//and then we keep on trying to match smaller and smaller strings
//If we try to match 'Attempt', we will match 'Attempt', 'Attemp' ,
//'ttempt', 'Attem', 'ttemp' etc. till 't' 
String.prototype.largestMatch = function largestMatch( otherString ){
 if( otherString.length < this.length )
 return otherString.largestMatch( this );
 
 var matchingLength = otherString.length,
 possibleMatch, index;
 while( matchingLength ){
 index = 0;
 while( index + matchingLength <= otherString.length ){
 possibleMatch = otherString.substr( index, matchingLength );
 if( ~this.indexOf( possibleMatch ) )
 return otherString.substr( index, matchingLength );
 index++;
 } 
 matchingLength--;
 }
 return '';
};
String.prototype.diff = function( newValue ){
 
 var largestMatch = this.largestMatch( newValue ),
 preNew, postNew, preOld, postOld;
 
 if(!largestMatch){
 return '<span class = "deleted">' + this + '</span><span class = "inserted">' + newValue + '</span>';
 } else {
 preNew = newValue.substr(0, newValue.indexOf( largestMatch ) ); 
 preOld = this.substr(0, this.indexOf( largestMatch ) ); 
 postNew = newValue.substr( preNew.length + largestMatch.length );
 postOld = this.substr( preOld.length + largestMatch.length );
 console.log( { old: this.toString(), new : newValue , preOld: preOld, match: largestMatch, postOld: postOld, 
 preNew: preNew, match2: largestMatch, postNew: postNew} );
 return preOld.diff( preNew ) + largestMatch + postOld.diff( postNew );
 }
};
textOld.oninput = function(e){textNew.innerText = this.value;};
textNew.onfocus = function(e){this.select();};
myButton.onclick = function(e){textdiff.innerHTML = textOld.value.diff(textNew.value);};

.deleted {background-color : LightPink;
 text-decoration : line-through}
.inserted {background-color : PaleGreen}

<!DOCTYPE html>
<html>
<head>
 <meta charset="utf-8">
 <meta name="viewport" content="width=device-width">
 <title>JS Bin</title>
</head>
<body>
 <div>
 <textarea id="textOld" placeholder="Please type something here" rows = "4" cols = "25">The quick brown fox jumps over the lazy dog</textarea>
 <textarea id="textNew" placeholder="Please edit the previous text here" rows = "4" cols = "25">The quick brown coyote jumps over the lazy dog</textarea>
 <button id = myButton style = "display:block"> Click to get diff</button>
 <p id="textdiff"></p>
</div>
 
 
</body>
</html>

I am going to attempt to write an alternative, dont hold your breath ;)

I wrote an alternative, it works slightly different. (I like that your code recognizes the word fox, whereas my code goes too far in finding commonalities) I think for an extra bonus, the code should go for both the largest match and the largest mismatch, whatever is largest should go forward. My code has some idiosyncracies, feel free to adopt or ignore (like using ~ with indexOf or not always using curly braces with if statements.)

//The idea is that we try to match the original string, 
//and then we keep on trying to match smaller and smaller strings
//If we try to match 'Attempt', we will match 'Attempt', 'Attemp' ,
//'ttempt', 'Attem', 'ttemp' etc. till 't' 
String.prototype.largestMatch = function largestMatch( otherString ){
 if( otherString.length < this.length )
 return otherString.largestMatch( this );
 
 var matchingLength = otherString.length,
 possibleMatch, index;
 while( matchingLength ){
 index = 0;
 while( index + matchingLength <= otherString.length ){
 possibleMatch = otherString.substr( index, matchingLength );
 if( ~this.indexOf( possibleMatch ) )
 return otherString.substr( index, matchingLength );
 index++;
 } 
 matchingLength--;
 }
 return '';
};
String.prototype.diff = function( newValue ){
 
 var largestMatch = this.largestMatch( newValue ),
 preNew, postNew, preOld, postOld;
 
 if(!largestMatch){
 return '<span class = "deleted">' + this + '</span><span class = "inserted">' + newValue + '</span>';
 } else {
 preNew = newValue.substr(0, newValue.indexOf( largestMatch ) ); 
 preOld = this.substr(0, this.indexOf( largestMatch ) ); 
 postNew = newValue.substr( preNew.length + largestMatch.length );
 postOld = this.substr( preOld.length + largestMatch.length );
 console.log( { old: this.toString(), new : newValue , preOld: preOld, match: largestMatch, postOld: postOld, 
 preNew: preNew, match2: largestMatch, postNew: postNew} );
 return preOld.diff( preNew ) + largestMatch + postOld.diff( postNew );
 }
};
textOld.oninput = function(e){textNew.innerText = this.value;};
textNew.onfocus = function(e){this.select();};
myButton.onclick = function(e){textdiff.innerHTML = textOld.value.diff(textNew.value);};

.deleted {background-color : LightPink;
 text-decoration : line-through}
.inserted {background-color : PaleGreen}

<!DOCTYPE html>
<html>
<head>
 <meta charset="utf-8">
 <meta name="viewport" content="width=device-width">
 <title>JS Bin</title>
</head>
<body>
 <div>
 <textarea id="textOld" placeholder="Please type something here" rows = "4" cols = "25">The quick brown fox jumps over the lazy dog</textarea>
 <textarea id="textNew" placeholder="Please edit the previous text here" rows = "4" cols = "25">The quick brown coyote jumps over the lazy dog</textarea>
 <button id = myButton style = "display:block"> Click to get diff</button>
 <p id="textdiff"></p>
</div>
 
 
</body>
</html>

Source Link

answered Jul 1, 2016 at 20:34

konijn

answered Jul 1, 2016 at 20:34

konijn

34.3k
5
70
267

Great question,

I do not like your rotate function;

While the nested ternary seems brilliant, it reveals a violation of DRY ( you could do this with one map because every negative n has a positive equivalent
An Array function should either change the current array or make a new array, your function could do either depending on whether a shift is necessary or not
Rotation really is achieved by taking a part of the string and putting it on the other end, since JavaScripts provides splice and shift I would go with something like this
Minor, but your indentation is off in this function and elsewhere, it disturbs the reading flow

I do like that you modify the prototype of Array. Most reviewers would complain but I have found 3rd party libraries to have sufficient guards nowadays that is no longer a problem.

Inside diff:

I see no good reason to declare getBaseIndex with var, even worse is that you declared it as an anonymous function. Naming your variables with s and l is not great, but getBaseIndex does not convey at all what the function actually does
match && (fi = i); // match starts from this index shows you know JavaScript, but really an if statement is what you should use here
Same here: s[i] !== l[i] ? ++i : match = !match;
From a naming perspective, spend the effort to have well written variables. bix, fis, fss, etc. etc. are too hard to parse for the reader
The commenting however, is great. Otherwise I would probably have given up on this review
isThisLonger = true; <- that semicolon just made all declarations under it globals (use http://jshint.com/)
I like the idea of matching largest matches first, not sure about rotating. If I was asked to fix a bug in this code, I would steal that smart key idea and rewrite the whole thing.

I am going to attempt to write an alternative, dont hold your breath ;)

default