Finding string duplicates

Question

I'm looking for ways to optimize a program in C++ I wrote to count all the duplicates in a string and print the duplicated letters and the number of times my program takes all 256 ASCII inputs and only prints the duplicates once with the max number of duplicates. I had a really hard time writing it and needed to ask for help and a few people had some things to say about my design. You can read it here if you like.

How can I write my design better? And what can I do in the future to write better programs in class and in the workplace?

#include<iostream>
#include<string>
#include<vector>
#include<algorithm>
using namespace std;
vector <char> alreadyprintedcharacters;
void findrepeats(string const&);
int main()
{
    string input;
    cout << "Enter the input : ";
    getline(cin, input);
    findrepeats(input);
    return 0;
}
void findrepeats(string const &in)
{
    int trackerOfDuplicates = 0; 
    int asciiArray[256];
    char ch;
    int charconv;
    for (int i = 0; i < 256; i++) // creates my refference array for the comparison and sets all the values equal to zero
        asciiArray[i] = 0;
    for (unsigned int i = 0; i < in.length(); i++)
    {
        ch = in[i];
        charconv = static_cast<int>(ch);
        asciiArray[charconv]++;
        /*if (asciiArray[charconv] == 0)
        {
            asciiArray[charconv] = 1;
        }
        else if (asciiArray[charconv] > 0)
        {
            asciiArray[charconv] = asciiArray[charconv]++;
        }*/

    }
    bool trip = false;
    for (unsigned int i = 0; i < in.length(); i++)
    {
        char static alreadyprinted;
        char ch = in[i];

        if ((asciiArray[ch] > 1) && (ch != alreadyprinted) && (find(alreadyprintedcharacters.begin(), alreadyprintedcharacters.end(), ch)== alreadyprintedcharacters.end()))
        {
            cout << in[i] << " : " << asciiArray[ch] << endl;
            trip = true;
            alreadyprinted = ch;
            alreadyprintedcharacters.push_back(alreadyprinted);
        }




    }
    if (trip == false)
        cout << "No repeated characters were found.\n";
}

Answer 1

A function does one thing, and one thing only.

Break your findrepeats() function apart. As the name indicates, only search for repeats, nothing else. Especially no output. When you are done, return asciiArray.

Maintain a clear separation of input and output, and the actual logic.

Keeping your functions short increases readability a lot, it also makes testing for bugs and restructuring your program later on a lot easier. In addition to that, it also reduces the number of local variables in a function, which aids further with readability.

---

int asciiArray[256];
for (int i = 0; i < 256; i++) // creates my refference array for the comparison and sets all the values equal to zero
    asciiArray[i] = 0;

Write it shorter:

int asciiArray[256] = {0};

Or use at least a builtin:

int asciiArray[256];
memset(asciiArray, 0, 256);

See also Array intialization

---

As mentioned in the other answer, don't try to iterate over the input string twice. You don't need to. asciiArray is all you need to generate the output, as long as the order doesn't matter.

---

vector <char> alreadyprintedcharacters;

Never use global variables. Just don't. That is bound to have side effects sooner or later.

Try e.g. calling findrepeats() twice now. The second call will not work as expected, as it is still (wrongly) using the exemption list from the first run.

This variable is only used inside findrepeats(), so put it in the local scope where it belongs.

---

int asciiArray[256];
char ch;

ch = in[i];
charconv = static_cast<int>(ch);
asciiArray[charconv]++;

Something is horribly wrong here, can you guess what?

Try to remember what char actually is. 256 distinct values, but was it 0..255 or was it rather -128..127?

You don't remember?
That's right. It's not defined what the default interpretation for a standalone char is, that depends on your compiler.

char alone is perfectly safe to transfer input to output, as signed or unsigned doesn't matter for that.

But if you want to use it as an array index, you must explicitly state that you want an unsigned char, or your program will crash horribly if the input contains anything outside the ASCII range (0..127), as it may be interpreted as a negative value.

Your compiler probably refused to accept char as an array index, and this problem is exactly the reason why. static_cast<int>(ch) only broke the type check, but your application will still crash.

Answer 2

The obvious question with respect to optimization is whether the order in which you print the duplicates matters--in particular, does it need to match the order in which those letters first appeared in the input, or can it be arbitrary?

If it doesn't need to match the order of the input string, you can simplify the algorithm quite a bit:

for each character in input string
    increment asciiArray[input]
for each character in asciiArray
    if count > 1 print character and count

This avoids the extra difficulty of things like having to keep track of whether you've already printed out a character/count--since you're walking through them in collation order, you only ever encounter each possible character once.