1

I wrote the following code for removing the duplicates from a given string i.e. if ARRUN is the input then the output will be ARUN.

#include <bits/stdc++.h>
using namespace std;
char* removeDuplicates(string &s,int n){
    char arr[n];
    unordered_map<char,int> exists;
    int index = 0;
    for(int i=0;i<n;i++){
        if(exists[s[i]]==0)
        {
            arr[index++] = s[i];
            exists[s[i]]++;
        }
    }
    return arr;
}

//driver code
int main(){
    string str;
    cin >> str;
    cout<<removeDuplicates(str,str.length())<<endl;
    return 0;
}

The code produces no output at all, however, it works fine if I use char arr[] instead of string class.

asmmo
  • 6,922
  • 1
  • 11
  • 25
Pikachu
  • 304
  • 3
  • 14
  • 4
    You cannot return an automatic (non static) array from a C++ function. Arrays are not first class objects in C++. – Serge Ballesta Feb 16 '20 at 11:07
  • @Arun Suryan Do you need to remove adjacent duplicate characters or all duplicate characters? – Vlad from Moscow Feb 16 '20 at 11:11
  • All duplicate characters. – Pikachu Feb 16 '20 at 11:13
  • 2
    You should *never* `#include `. It is not just bad practice - it is not proper C++. It ruins portability and fosters terrible habits. By using it, you not only grant the compiler the right to break your code at any time without notice but also renders your code non-portable and unprofessional. It also creates implicit dependency on any future facility of the C++ standard library, thus basically screwing up compile time. See [Why should I not `#include `](https://stackoverflow.com/q/31816095). – L. F. Feb 16 '20 at 12:34

8 Answers8

4

You can't use char arr[n] without being n constant or constexpr.

You don't need map. set is sufficient.

Note that map and set remove duplicates already, then you can check if any element is inserted or not to get your new string in the same order of the first, as follows

#include<string>
#include<iostream>
#include<unordered_set>

std::string removeDuplicates(const std::string &s){
    std::string arr;
    std::unordered_set<char> exists;

    for(const auto&el:s)
        if(exists.insert(el).second) arr+=el;

    return arr;
}

//driver code
int main(){
    std::string str;
    std::cin >> str;
    std::cout<<removeDuplicates(str)<<std::endl;
    return 0;
}
asmmo
  • 6,922
  • 1
  • 11
  • 25
  • I loved your coding style. But I am just too lazy to write `std::` again and again. `bits/stdc++.h` gets the job done. Though, I know it's not a good practice. – Pikachu Feb 16 '20 at 11:27
  • @ArunSuryan except only one compiler line got that header – Swift - Friday Pie Feb 16 '20 at 11:54
  • 1
    @ArunSuryan *bits/stdc++.h gets the job done* -- Please read [this](https://stackoverflow.com/questions/31816095/why-should-i-not-include-bits-stdc-h) – PaulMcKenzie Feb 16 '20 at 11:59
  • It's very crucial in sports programming. That's why I often use it. It saves typing effort. – Pikachu Feb 16 '20 at 12:02
  • 1
    Also [read this](https://stackoverflow.com/questions/58423310/why-is-an-error-popping-out-when-i-am-trying-to-use-priority-queue-with-paramete/58423503#58423503). Your "sports programming" header file will fail miserably if you ever write code that uses `data` as a struct type or similar. – PaulMcKenzie Feb 16 '20 at 12:22
  • @ArunSuryan "Sports programming" forces you into bad programming habits and teaches you virtually none of the skills you need in practical programming. It is like learning English from a rap contest (sorry Lightness) - all it does for you is generate and reinforce misunderstandings of C++ (and programming in general), especially at an early stage when you are not familiar with C++ at all. We are telling you that what you are writing is not proper and should not be used; if you try to find an excuse as ridiculous as "crucial in sports programming," then you are blatantly dismissing our effort. – L. F. Feb 17 '20 at 00:55
  • So, you learn Data Structures, better algorithms, smart use of time and memory. `you virtually none of the skills you need in practical programming` I can't believe you just said this. – Pikachu Feb 17 '20 at 19:20
2

std::string support removing elements.

#include <iostream>
#include <string>

std::string removeDuplicates(std::string str) {
    for (int i = 0; i < str.size(); i++) {
        while (true) {
            int j = str.find_last_of(str[i]);
            if (i < j) {
                str.erase(j, 1);
            } else {
                break;
            }
        }
    }
    return str;
}

int main() {
    std::cout << removeDuplicates("ARRUN");
    return 0;
}
Onyrew
  • 126
  • 7
1

@Arun Suryan, You pointed out correctly. But you can do it without using vector by using global char array.

Also don't forget to append the newline at the end!

Have a look at the following code:

#include<string>
#include<iostream>
#include<unordered_map>

char* removeDuplicates(std::string &s,int n){

    std::unordered_map<char,int> exists;
    char* arr = (char*)(malloc(n*sizeof(char)));
    int index = 0;
    for(int i=0;i<n;i++){
        if(exists[s[i]]==0)
        {
            arr[index++] = s[i];
            exists[s[i]]++;
        }
    }
    arr[index] = '\n';
    return arr;
}

//driver code
int main(){
    std::string str;
    std::cin >> str;
    std::cout<<removeDuplicates(str,str.length())<<std::endl;
    return 0;
}
Deepak Tatyaji Ahire
  • 4,883
  • 2
  • 13
  • 35
  • Yes you can, but returning values through global mutables without real need to do so is Fortran-77 style of programming (because Fortran had no other way). vector uses heap, which is global but symbolically the manager object got certain locality and identity. In large projects use of global static mutables becomes a real menace which is pointed out by C++ and similar imperative languages opponents. – Swift - Friday Pie Feb 16 '20 at 11:27
  • @Swift-FridayPie Agreed! – Deepak Tatyaji Ahire Feb 16 '20 at 11:31
  • Sorry, I have to downvote your answer, because it promotes the usage of global variables and raw C arrays with fixed size (thus leading to a guaranteed buffer overflow), neither of which is compatible with modern C++ programming practices. Furthermore, this approach significantly increases the difficulty of code analysis ("Did some function just modify that global variable?"), letting alone introducing data races. Consistently use `std::string` and value semantics instead. And that not even counting `#include ` `using namespace std;`. – L. F. Feb 16 '20 at 12:39
  • @L.F. No worries! Suggestions taken! – Deepak Tatyaji Ahire Feb 16 '20 at 13:03
  • Great. You can improve the answer by [edit]ing it, so I can retract my downvote. – L. F. Feb 16 '20 at 13:13
  • @L.F. Improved my answer. Thanks for suggestions – Deepak Tatyaji Ahire Feb 16 '20 at 14:32
  • 1
    `malloc` is just as bad. You should be using `std::string` instead of a manually allocated (or fixed sized) `char` array. In fact you are leaking the allocation right now and `malloc` without placement-new is technically (currently) always undefined behavior. There is no reason to use it over `new[]`. – walnut Feb 16 '20 at 15:20
1

If a function declaration looks the following way

char* removeDuplicates(string &s,int n);

then it means that the passed object itself will be changed in the function. Otherwise the parameter shall have the qualifier const.

Also it is unclear why the function has return type char *. It looks like the declaration of the function is contradictive.

The second parameter of the function shall have at least the type size_t or that is better std::string::size_type. The type int can not accomodate all values of the type std::string::size_type.

The function could be declared without the second parameter.

A straightforward approach without using intermediate containers that requires dynamic memory allocation can look the following way

#include <iostream>
#include <string>

std::string & removeDuplicate( std::string &s )
{
    const char *p = s.c_str();

    std::string::size_type pos = 0;

    for ( std::string::size_type i = 0, n = s.size(); i < n; i++ )
    {
        std::string::size_type j = 0;
        while ( j < pos && s[i] != s[j] ) j++;

        if ( j == pos )
        {
            if ( i != pos ) s[pos] = s[i];
            ++pos;
        }
    }

    return s.erase( pos );
}

int main() 
{
    std::string s( "H e l l o" );

    std::cout << "\"" << s <<"\"\n";

    std::cout << "\"" << removeDuplicate( s ) <<"\"\n";

    return 0;
}

The program output is

"H e l l o"
"H elo"
Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
1

This might be a bit advanced for newcomers to C++ but another solution makes use of the erase-remove idiom:

std::string removeDuplicates(const std::string& s) {
    std::string result = s;
    std::unordered_set<char> seen;

    result.erase(std::remove_if(result.begin(), result.end(), [&seen](char c)
        {
            if (seen.find(c) != seen.end())
                return true;

            seen.insert(c);
            return false;
        }),
    result.end());

    return result;
}

It basically uses a set to store characters that have been seen, shuffles the characters to be removed down to the tail (using std::remove_if) and erases the tail from the string.

Working version here.

jignatius
  • 6,304
  • 2
  • 15
  • 30
1

This works too, a single line solution with an inbuild function.

cout<<str.erase(std::unique(str.begin(), str.end()), str.end());

0

Simple Answer

#include<bits/stdc++.h>
using namespace std;
string removeduplicate(string key){
    set<char>s;
    string ans="";
    for(int i=0;i<key.size();++i){
        if(s.find(key[i])==s.end()){
            s.insert(key[i]);
            ans.push_back(key[i]);
        }
    }
    return ans;

}



int main()
{
    string key="";
    cout<<"enter the key:";
    cin>>key;
    string ans1=removeduplicate(key);
    cout<<ans1;
    return 0;
}
-1

So, after doing a bit of reading on the Internet I realized that I was trying to return a pointer to the local array in the removeDuplicates() function.

This is what works fine

#include <bits/stdc++.h>
using namespace std;
void removeDuplicates(string &s,int n){
    vector<char> vec;
    unordered_map<char,int> exists;
    int index = 0;
    for(int i=0;i<n;i++){
        if(exists[s[i]]==0)
        {
            vec.push_back(s[i]);
            exists[s[i]]++;
        }
    }
    for(auto x: vec)
        cout << x;
}

//driver code
int main(){
    string str;
    cin >> str;
    removeDuplicates(str,str.length());
    return 0;
}

PS: We can make the return type of function to vector as well.

Pikachu
  • 304
  • 3
  • 14