Finding string duplicates

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
#include
#include
#include
using namespace std;
vector  alreadyprintedcharacters;
void findrepeats(string const&);
int main()
{
    string input;
    cout (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  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";
}

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  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(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(ch) only broke the type check, but your application will still crash.

StackExchange Code Review Q#125960, answer score: 2