Talk:Algorithm Implementation/Strings/Longest common subsequence

Error in the section "C# - Reading the LCS
I think there is an error in this section, because if s1position and/or s2position are exactly zero, and the corresponding char in string1 doesn't match the char in string2, then the algorithm will try to access a negative index in the backtrack array which should result in an indexOutOfBounds exception.

Therefore I would suggest to add the condition below after the

statement:

Additionally "s2posision" should probably renamed to "s2position" to be more meaningful. --87.95.15.51 (talk) 17:37, 28 July 2009 (UTC) I'm sure there is another problem - it is not really reading data from 2d array from the first example. It needs to delete first row and first column in the array. Or should be replaced with code

Call will look like

(79.164.26.4 (discuss) 21:10, 31 July 2011 (UTC))

The condition suggested above doesn't works (for input string "Too" and "To" the result would be "o", missing the first letter "T")

This could be a solution

--Millsabord (discuss • contribs) 09:48, 18 February 2012 (UTC)

Actually, that is not a solution. The problem comes from the fact that "backtrack" and "string1" and "string2" use different indexing. The "backtrack" matrix has an additional row and column. One way to solve this is to use different indices for the "backtrack" matrix:

Are the null checks in the Java implementation necessary and correct?
It's not obvious that the null checks in the Java implementation here are necessary or correct:

protected boolean equals(VALUE x1, VALUE y1) { return (null == x1 && null == y1) || x1.equals(y1); }

If x1 can be null when y1 is non-null, then x1.equals(y1) can throw a NullPointerException and a test like this should be used instead:

protected boolean equals(VALUE x1, VALUE y1) { return x1 != null ? x1.equals(y1) : y1 == null; }

If x1 can be null only when y1 is also null, then the code is ok but a comment would be appropriate because that's not obvious.

If neither should be null, then the null checks should be removed since they could cover up a coding error, and equals shouldn't be quietly accepting invalid arguments. Atomota 23:09, 22 November 2007 (UTC)

Redundant assignment in LongestCommonSubsequence.backtrack
This has no effective impact on the algorithm implementation, but the following line in the LongestCommonSubsequence.backtrack method:

this.backtrack = backtrack = new ArrayList;

could be simplified. The middle term is redundant, resulting in:

this.backtrack = new ArrayList;

(I noticed this when Eclipse pointed it out to me.)

C++ example allocating a 2-D array
For the C++ example I deliberately left out the code for creating a 2-D array to keep things simple, and because everyone has a different way they like to do this. Here is one possible way to create a 2-D array that matches the example code:

The "result" variable is not properly initialized in the C/C++ listing
Multiple calls to findOne in the following style results in garbage like: r auli of length 6 is the LCS between British auxilia and Kangaroo (armoured personal carrier) r aulit of length 8 is the LCS between British auxilia and Ram tank Mk I r aulit t  of length 10 is the LCS between British auxilia and Ram tank Mk II r aulit t  aa of length 13 is the LCS between British auxilia and Ram Kangaroo i.m.h.o., result variable is not initialized. Adding result.clear at start of backtrackOne do help