HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

First non-repeated character

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
firstcharacterrepeatednon

Problem

Challenge description is here need to find the first non repeated character from the string:

Write a program which finds the first non-repeated character in a string.

Input sample:

The first argument is a path to a file. The file contains strings.

For example:

yellow
tooth


Output sample:

Print to stdout the first non-repeated character, one per line.

For example:

y
t


Please let me know can I improve my solution.

#include 
#include 
#include 
#include 
#include 

 
void findNonRepeatedChar( const std::string& record)
{
    auto pred= [&record] ( char a, char b){ 
                  return strchr( record.c_str(), a )  stringMap(pred);
    for( auto elem : record )
    {
        auto it = stringMap.find( elem );
        if( it != std::end(stringMap) )
        {
            ++(it->second);
        }
        else
        {
            stringMap.insert( std::make_pair(elem, 1));
        }
    }
    for( auto elem : stringMap)
    {
        if(elem.second == 1)
        {
            std::cout<< elem.first << "\n";
            break;
        }
    }
}

void readInputFile( const char * fileName )
{
    std::ifstream inFile( fileName, std::ifstream::in );
    std::string record="tooth";
    
    if( !inFile.is_open())
    {
        std::cout<<"File open failed \n";
        return;
    }
    while( getline( inFile, record ) )
    {
        findNonRepeatedChar(record);
    }
}

int main( int argc, char* argv[] )
{
    if( argc < 2 )
    {
        std::cout<<"Usage:program_name input_file_name \n";
    }
    readInputFile(argv[1]);
}

Solution

I've reviewed your code and here's what I've found.

Reconsider your algorithm

It's not really necessary to iterate through all of the characters in the string, as the current code does. Since the task is to find only the first non-repeated character, the algorithm can terminate as soon as it finds a non-repeated character. A simple way to do that is this:

void findNonRepeatedChar( const std::string& record )
{
    for (std::string::size_type i = 0; i < record.size(); ++i) {
        if (record.rfind(record[i]) == i) {
            if (record.find(record[i]) == i) {
                std::cout << record[i] << '\n';
                return;
            }
        }
    }
}


Reconsider the interface

It is odd that a function named findNonRepeatedChar() does not actually return anything. The code could be clearer if it actually returned the non-repeated letter and left the printing to the calling routine.

Don't instantiate an object that is not needed

There is no reason to instantiate the record to "tooth" (or any other value) within readInputFile.

Use more descriptive function names

The function named readInputFile does more than simply read the file and as mentioned before, the findNonRepeatedChar() function doesn't return anything but prints as a side effect. I'd be inclined to omit readInputFile entirely, and put its contents within main and to change the interface to as previously mentioned.

Give the user useful help

The program is probably not really named program_name so the string that's printed if there aren't enough commmand line arguments should probably instead be:

std::cout << "Usage: " << argv[0] << " input_file_name\n";

Code Snippets

void findNonRepeatedChar( const std::string& record )
{
    for (std::string::size_type i = 0; i < record.size(); ++i) {
        if (record.rfind(record[i]) == i) {
            if (record.find(record[i]) == i) {
                std::cout << record[i] << '\n';
                return;
            }
        }
    }
}
std::cout << "Usage: " << argv[0] << " input_file_name\n";

Context

StackExchange Code Review Q#92352, answer score: 6

Revisions (0)

No revisions yet.