CODEWITHSUNDEEP
c++c-stringsfunction-definitiontoupper
Clancinio
Clancinio

Reputation: 942

Passing a pointer into methods in C++ resulting in weird output

I am trying to convert a string of characters to Upper case by passing a pointer into a toUpper method that I have created. The logic seems to be fine but I get a weird output like ìëïà. Any ideas where I have gone wrong here?

#include <iostream>
#include <string.h> 

using namespace std;

void toUpper(char *);

int main()
{
    char name[80];
    char *namePtr = name;

    cout << "Enter a name :";
    cin >> name;

    toUpper(namePtr);
    cout << "The string in Upper Case is: " << name << endl;


}

void toUpper(char *p)
{

    int asciiValue; 

    // Loop through each char in the string
    for(int i = 0 ; i < strlen(p); i++)
    {
        
        asciiValue = (int) p[i];

        if(asciiValue >= 97 && asciiValue <= 122)
        {
            asciiValue = asciiValue + 32;
            p[i] = asciiValue;
        }
    }

}

Upvotes: 1

Views: 229

Answers (5)

Christophe
Christophe

Reputation: 73456

Your code is non portable

This is non portable code. It is sure to work only with ASCII encoding. Nevertheless, here the corresponding non-portable solution:

       asciiValue = asciiValue - 32;   // - just move in the other direction

How to do better?

Here some problems for your current code:

  • The difference between upper and lowercase letters is not always 32. In EBCDIC, for example, the difference between upper and lowercase ordinary letters is +64 instead of -32.
  • The boundaries of lowercase might be different as well.
  • With foreign languages using non ASCII locale, you might have special characters that are in a different range than the normal letters (see for example ISO-8859-1) where you also have lowercase in the range 224 and 25' but with one exception.
  • In some encodings, you have even different rules for different sets of lowercases. Take ISO 8859-3. The difference between 'Ŭ' and 'ŭ' is -32 but between 'Ż' and 'ż' it's -16 .
  • Finally, chars are not guaranteed to be unsigned. Your whole comparison logic might completely fail, if you use an ISO-8859-1 encoding compined with a compiler that manages chars as signed chars.

A much safer approach is therefore to to use: isupper() and toupper() which takes into account the locale.

As a side effect, this will even facilitate migration to a full unicode compliant code, using the templated version of these functions or the wide version.

And why don't you use real strings?

Your code is at risk of a buffer overflow, if someone types a name of 80 or more characters. You'd need to make sure that cin does not take more characters than allowed. But instead of telling you how to do this, I suggest to use the safer std::string instead:

void toUpper(string &s)
{
    for(auto &p:s)              // Loop through each char in the string
        if (islower(p))         
            p =toupper(p);
}

int main()
{
    string name;
    cout << "Enter a name :";
    cin >> name;
    toUpper(name);
    cout << "The string in Upper Case is: " << name << endl;
}

online demo

Upvotes: 3

Thomas Matthews
Thomas Matthews

Reputation: 57728

A more portable C++ solution is to use std::transform to transform the string to lower case:

std::string shouting = "AM I SHOUTING";
std::transform(shouting.begin(), shouting.end(), shouting.begin(), tolower);
std::cout << shouting << "\n";

This solution does not rely on ASCII encoding and will work with code sets where std::tolower is valid.

Upvotes: 0

Vlad from Moscow
Vlad from Moscow

Reputation: 311068

It seems in this if statement

    if(asciiValue >= 97 && asciiValue <= 122)
    {
        asciiValue = asciiValue + 32;
        p[i] = asciiValue;
    }

you are checking that the current symbol is a lower-case ASCII symbol.

But lower-case ASCII symbols have higher codes than upper-case ASCII symbols.

So instead of adding the magic number 32

        asciiValue = asciiValue + 32

you have to subtract it

        asciiValue = asciiValue - 32

For example the lower case ASCII symbol 'a' has the code 97 while the upper case.symbol 'A' has the code 65.

But in any case your approach with magic numbers is bad because for example it will not work with EBCDIC symbol representations.

Also calling the function strlen in this case is inefficient.

And it is much better when the function returns pointer to the converted string.

The function can be declared and implemented the following way

#include <cctype>

//...

char * toUpper( char *s )
{
    for ( char *p = s; *p; ++p )
    {
        if ( std::islower( static_cast<unsigned char>( *p ) ) )
        {
            *p = std::toupper( static_cast<unsigned char>( *p ) );
        }
    }

    return s;
}

Upvotes: 1

Juned Khan
Juned Khan

Reputation: 122

asciiValue = asciiValue - 32;

Minus instead of plus Example: ASCII value of "a" is 97

97 - 32 is 65 which is the ASCII value of uppercase A

Upvotes: 1

scohe001
scohe001

Reputation: 15446

Your problem boils down to bad magic numbers, which makes it nearly impossible to tell even from a close look because they're magic numbers!

Instead, I'd use character literals to make things obvious:

if(asciiValue >= 'a' && asciiValue <= 'z')
{
    asciiValue = asciiValue + ('a' - 'A');
    p[i] = asciiValue;
}

Now it should be pretty apparent that you're adding the wrong value! It should instead be:

asciiValue = asciiValue + ('A' - 'a');

Upvotes: 3

Related Questions