Finding and printing common letters in different strings

Question

I need to find and print common characters in different strings. My code does not work as it should, it checks for same letters at the same index but thats not what I want. I couldn't find better solution for now. Thank you for help :)

#include <iostream>
#include <string>
using namespace std;

int main() {
    string niz1, niz2, niz3 = "";
    cout << "string 1: ";
    getline(cin, niz1);
    cout << "string 2: ";
    getline(cin, niz2);

    for (int i = 0; i < niz1.length() - 1; i++) {
        for (int j = 0; j < niz2.length() - 1; j++) {
            if (niz1[i] == niz2[j])
                niz3 += niz1[i];
        }
    }

    cout << "Same letters are: " << niz3 << endl;
    return 0;
}

Answer 1

Below is corrected working code. Basically the only correction that was needed to do is to make both loops have upper bound of niz.length() instead of niz.length() - 1.

Variant 1:

Try it online!

#include <string>
#include <iostream>
using namespace std;

int main() {
    string niz1, niz2, niz3 = "";
    cout << "string 1: ";
    getline(cin, niz1);
    cout << "string 2: ";
    getline(cin, niz2);

    for (int i = 0; i < niz1.length(); i++) {
        for (int j = 0; j < niz2.length(); j++) {
            if (niz1[i] == niz2[j])
                niz3 += niz1[i];
        }
    }

    cout << "Same letters are: " << niz3 << endl;

    return 0;
}

Input:

string 1: adbc
string 2: cde

Output:

Same letters are: dc

Also you may want to sort letters and make them unique, then you need to use std::set too like in code below:

Variant 2:

Try it online!

#include <string>
#include <iostream>
#include <set>
using namespace std;

int main() {
    string niz1, niz2, niz3 = "";
    cout << "string 1: ";
    getline(cin, niz1);
    cout << "string 2: ";
    getline(cin, niz2);

    for (int i = 0; i < niz1.length(); i++) {
        for (int j = 0; j < niz2.length(); j++) {
            if (niz1[i] == niz2[j])
                niz3 += niz1[i];
        }
    }

    set<char> unique(niz3.begin(), niz3.end());
    niz3.assign(unique.begin(), unique.end());

    cout << "Same letters are: " << niz3 << endl;

    return 0;
}

Input:

string 1: adbcda
string 2: cdecd

Output:

Same letters are: cd

Also you may use just sets plus set_intersection standard function. This will solve your task in less time, in O(N*log(N)) time instead of your O(N^2) time.

Variant 3:

Try it online!

#include <string>
#include <iostream>
#include <set>
#include <algorithm>
#include <iterator>
using namespace std;

int main() {
    string niz1, niz2, niz3 = "";
    cout << "string 1: ";
    getline(cin, niz1);
    cout << "string 2: ";
    getline(cin, niz2);

    set<char> s1(niz1.begin(), niz1.end()), s2(niz2.begin(), niz2.end());
    set_intersection(s1.begin(), s1.end(), s2.begin(), s2.end(), back_inserter(niz3));

    cout << "Same letters are: " << niz3 << endl;

    return 0;
}

Input:

string 1: adbcda
string 2: cdecd

Output:

Same letters are: cd

Instead of set it is also possible to use unordered_set, it will give even more faster algorithm especially for long strings, algorithm will have running time O(N) compared to O(N * log(N)) for set solution. The only drawback is that unlike for set solution output of unordered_set solution is unsorted (but unique) (unordered sets don't sort their data).

Variant 4:

Try it online!

#include <string>
#include <iostream>
#include <unordered_set>
#include <algorithm>
#include <iterator>
using namespace std;

int main() {
    string niz1, niz2, niz3;
    cout << "string 1: ";
    getline(cin, niz1);
    cout << "string 2: ";
    getline(cin, niz2);

    unordered_set<char> s1(niz1.begin(), niz1.end()), s2;
    for (size_t i = 0; i < niz2.length(); ++i)
        if (s1.count(niz2[i]))
            s2.insert(niz2[i]);
    niz3.assign(s2.begin(), s2.end());

    cout << "Same letters are: " << niz3 << endl;

    return 0;
}

Input:

string 1: adbcda
string 2: cdecd

Output:

Same letters are: dc

Also one more way is to use just plain for loops like you did, without sets, but do extra block of loops in order to remove non-unique letters, like in code below. The only drawbacks of this loops method compared to sets method is that loops method runs slower and produces non-sorted output string.

Variant 5:

Try it online!

#include <string>
#include <iostream>
using namespace std;

int main() {
    string niz1, niz2, niz3, niz4;
    cout << "string 1: ";
    getline(cin, niz1);
    cout << "string 2: ";
    getline(cin, niz2);

    for (int i = 0; i < niz1.length(); ++i)
        for (int j = 0; j < niz2.length(); ++j)
            if (niz1[i] == niz2[j])
                niz3 += niz1[i];
    
    for (int i = 0; i < niz3.length(); ++i) {
        bool exists = false;
        for (int j = 0; j < niz4.length(); ++j)
            if (niz4[j] == niz3[i]) {
                exists = true;
                break;
            }
        if (!exists)
            niz4 += niz3[i];
    }

    cout << "Same letters are: " << niz4 << endl;

    return 0;
}

Input:

string 1: adbcda
string 2: cdecd

Output:

Same letters are: dc

Answer 2

This is sort of an answer in itself, and sort of an extended comment on @Arty's answer.

Hash tables (which are what underlies an unordered_map) are really useful under many circumstances. But in this case, they're kind of overkill. In particular, a hash table is basically a way of creating a sparse array for cases where it's unrealistic or unreasonable to use the underlying "key" type directly as an index into an array.

In this case, however, what we're using as the key in the hash table is a character--a single byte. This is small enough, it's utterly trivial to just use an array, and use the byte directly as an index into the array.

So, with arrays instead of hash tables, we get code something on this order:

#include <array>
#include <string>
#include <iostream>
#include <chrono>

std::string getstring(std::string const &s) {
    std::cout << s << ": ";
    std::string input;
    std::getline(std::cin, input);
    return input;
}

using namespace std::chrono;

int main() {

    std::array<char, 256> a = {0};
    std::array<char, 256> b = {0};
    std::array<char, 256> result = { 0 };
    std::size_t pos=0;

    std::string s1 = getstring("s1");
    std::string s2 = getstring("s2");

    std::cout << "s1: " << s1 << "\n";
    std::cout << "s2: " << s2 << "\n";

    auto start = high_resolution_clock::now();

    for (auto c : s1)
        a[c] = 1;
    for (auto c : s2)
        b[c] = 1;

    for (int i = 'a'; i < 'z'; i++)
        if (a[i] != 0 && b[i] != 0)
            result[pos++] = i;
    for (int i = 'A'; i < 'Z'; i++)
        if (a[i] != 0 && b[i] != 0)
            result[pos++] = i;

    auto stop = high_resolution_clock::now();

    std::cout << "Common characters: " << std::string(result.data(), pos) <<"\n";
    std::cout << "Time: " << duration_cast<nanoseconds>(stop - start).count() << " nS\n ";
}

To get some repeatable test conditions, I built an input file with a couple of long fairly strings:

asdffghjllkjpoiuwqertqwerxvzxvcn
qqweroiglkgfpoilkagfskeqwriougfkljzxbvckxzv

After adding instrumentation (timing code) to his Variant 4 code, I found that his code ran in about 12,000 to 12,500 nanoseconds.

The code above, on the other hand, runs (on the same hardware) in about 850 nanoseconds, or around 15 times as fast.

I'm going to go on record as saying the opposite: although this is clearly faster, I'm pretty sure it's not the fastest possible. I can see at least one fairly obvious improvement (store only one bit per character instead of one byte) that would probably improve speed by at least 2x, and could theoretically yield an improvement around 8x or so. Unfortunately, we've already sped other things up enough that I doubt we'd see 8x--we'd probably see a bottleneck on reading in the data from memory first (but it's hard to be sure, and likely to vary between processors). So, I'm going to leave that alone, at least for now. For now, I'll settle for only about fifteen times faster than "the fastest possible solution"... :-)

I suppose, in fairness, he probably really meant asymptotically the fastest. His has (expected, but not guaranteed) O(N) complexity, and mine also has O(N) complexity (but in this case, basically guaranteed, not not just expected. In other words, the 15x is roughly a constant factor, not one we expect to change significantly with the size of the input string. Nonetheless, even if it's "only" a constant factor, 15x is still a pretty noticeable difference in speed.