-3

Here is the question statement

Given two strings s1 and s2, return the length of their longest common subsequence.

A subsequence of a string is a new string generated from the original string with some characters(can be none) deleted without changing the relative order of the remaining characters. (eg, "ace" is a subsequence of "abcde" while "aec" is not). A common subsequence of two strings is a subsequence that is common to both strings.

If there is no common subsequence, return 0.

Here is my code

class Solution {
public:
    int dp[1001][1001];
    int lcs(string s1,string s2,int n,int m){
        if (n==0 || m == 0)
            return 0;
        if(dp[n][m]!=-1)
            return dp[n][m];
        if (s1[n-1] == s2[m-1])
        {
            return dp[n][m]=  1 + lcs(s1,s2,n-1,m-1);
        }
        else
        {
            return dp[n][m]= max(lcs(s1,s2,n-1,m), lcs(s1,s2,n,m-1));
        }
        return dp[n][m];
    }
    int longestCommonSubsequence(string s1, string s2) {
        memset(dp,-1,1001*1001*sizeof(int));
        return lcs(s1,s2, s1.size(), s2.size());
        
    }
};

Thank You

Nishant Sagar
  • 41
  • 5
  • 1
    Do you need to pass by value all the strings? – Bob__ Oct 24 '20 at 08:33
  • @Nishant Sagar always use `memset(dp, -1, sizeof (dp))` because you may change size of` dp` or datatype of `dp` it may give an unexpected result. – pradeexsu Oct 24 '20 at 08:44
  • 1
    BTW, are you sure this is the correct algorithm? When passed `"abcde"` and `"ba"`, the function returns 2. Shouldn't it be 1, by the question statement? – Bob__ Oct 24 '20 at 09:23
  • 2
    I supposed by TLE you mean Time Limit Exceeded, but the world does not proft infinitely from an endless proliferation of TLAs. – user207421 Oct 24 '20 at 09:28
  • 1
    `return dp[n][m];` will never execute at end of `lcs` function. – pradeexsu Oct 24 '20 at 09:35
  • 1
    @Bob__ its not the longest common substring, see https://en.wikipedia.org/wiki/Longest_common_subsequence_problem – Surt Oct 24 '20 at 10:01

2 Answers2

2

use call by reference in function definition instead of call by value.

int lcs(string &s1,string &s2,int n,int m);

int longestCommonSubsequence(string &s1, string &s2);
pradeexsu
  • 1,029
  • 1
  • 10
  • 27
0

Your implementation has at least 2 further improvements that can be made.

  • You are using a recursive algorithm, rewrite it to an iterative version
  • You are using a 1001*1001 array and therefor you are trashing the cache to some extend

There exists a much more space efficient algorithm, Hirschberg's algorithm, for just returning the length of the LCS.

Surt
  • 15,501
  • 3
  • 23
  • 39