Creating a custom uppercase method

Question

I'm trying to create an upper case method(i can't use it from library cause my software doesn't support it). The problem is that when i use my method in my output have always the last result from my strings. I can't really understand where exactly is the problem. I believe that i don't handle pointers with right way.

Here is an example:

Into initialize :

char *Register[5];

Inside my while :

char *p;
int i =0;
for(i=0;i<=4;i++)
{
    if(i==0)p="test1";
    if(i==1)p="test2";
    if(i==2)p="test3";
    if(i==3)p="test4";
    Register[i]=ToUpper(p);
}
Eusart2_Write(Register[0]);
__delay_ms(20);
Eusart2_Write(Register[1]);
__delay_ms(20);
Eusart2_Write(Register[2]);
__delay_ms(20);
Eusart2_Write(Register[3]);

And here is my upper method :

char *ToUpper(char *string)
{
    int i=0;
    char txt[255]="";
    char Buffer[255]="";

    strcpy(Buffer,string);

    for(i = 0; i<=strlen(Buffer); i++)
    {
        if(( Buffer[i]>='a')&&( Buffer[i]<='z'))
            txt[i]=Buffer[i] - 32; 
        else
            txt[i]= Buffer[i];  
    }
    txt[i++]='\0';

    return txt;
}

In my output I'm taking the same result for all registers :

TEST4TEST4TEST4TEST4

You have multiple problems with the code you show, the most important being that it's not a [Minimal, Complete, and Verifiable Example](http://stackoverflow.com/help/mcve). Also please [read about how to ask good questions](http://stackoverflow.com/help/how-to-ask) and [this question checklist](https://codeblog.jonskeet.uk/2012/11/24/stack-overflow-question-checklist/). — Some programmer dude, Sep 10 '18 at 07:28
However to ***guess*** what's happening, is that you have a *single array* `txt`, and you assign all pointers in `Register` to point to the first element of this single array. I.e. all pointers will point to the very same location. — Some programmer dude, Sep 10 '18 at 07:33
One of many problems: Your Register array has 5 elements, but you treat it like it has 21... — Shawn, Sep 10 '18 at 07:37
I always wince when someones implements an O(n**2) solution to an O(n) problem. — Tom Karzes, Sep 10 '18 at 07:39
@TomKarzes: Indeed, although some compilers are smart enough to optimise out the repeated `strlen` calls, in this case there are hints that it will not. — Bathsheba, Sep 10 '18 at 07:40
Instead of making an array of pointers, make it an array of *arrays*. Then copy the string into `Register[i]`, and modify your `ToUpper` function to modify the string in place. — Some programmer dude, Sep 10 '18 at 07:41
And a note about style: Stop using [*magic numbers*](https://en.wikipedia.org/wiki/Magic_number_(programming)). If by e.g. `97` you mean the ASCII encoding for `'a'` then *say so*. Of better yet, use the *standard* [`toupper`](https://en.cppreference.com/w/c/string/byte/toupper) function. — Some programmer dude, Sep 10 '18 at 07:42
I fix it in my question. But why it writes always the same location? — Dhi, Sep 10 '18 at 07:47
Possible duplicate of [Returning an array using C](https://stackoverflow.com/questions/11656532/returning-an-array-using-c) — Sander De Dycker, Sep 10 '18 at 07:50

P.W · Accepted Answer · 2018-09-10T10:26:25.120

0

Your txt array is local to the function ToUpper. Once it goes out of scope, you cannot guarantee what its contents will be. So if your Register variable is global, you can also make the txt array global as well.

Second, you are writing only after converting ALL the strings to uppercase. So even if your txt array was global, it will only contain what is written most recently into it (which is "TEST4"). The solution to this is to write immediately after conversion. So move the write into the for loop like this:

for(i=0;i<=4;i++)
{
    if(i==0)p="test1";
    if(i==1)p="test2";
    if(i==2)p="test3";
    if(i==3)p="test4";
    Register[i]=ToUpper(p);
    Eusart2_Write(Register[i]);
    __delay_ms(20);
}

Note: You have only four strings in the for loop which has 5 iterations, so when you write you will end up writing TEST4 twice, because p will retain "test4" when i equals 4. So you have to correct the controlling expression in the for loop as well.

However, if you want to retain the values in Register and print them after the for loop, then you have to make Register a 2 dimensional array and copy the return value of ToUpper into it.

Your register declaration will look like this:

char Register[5][6];

and your for loop will look like this:

for(i=0;i<4;i++)
{
    if(i==0)p=(char*)"test1";
    if(i==1)p=(char*)"test2";
    if(i==2)p=(char*)"test3";
    if(i==3)p=(char*)"test4";
    strcpy(Register[i], ToUpper(p));
    printf("%s\n", &Register[i]); 
}

See this example.

edited Sep 10 '18 at 10:26

answered Sep 10 '18 at 08:10

P.W

26,289
6
39
76

Thank you for your answer but with your method, all registers will have the same name. I will be able to print them right, but when i will use them inside my code i will have the same name in all registers – Dhi Sep 10 '18 at 08:26
I didn't get your comment. You mean that all registers are printing TEST4? I tested the code I posted and it printed correctly. – P.W Sep 10 '18 at 08:39
Yes if i will use Register[2] after my loop it prints TEST4 – Dhi Sep 10 '18 at 08:46
Here is the code, you can see the output, it prints correctly: https://coliru.stacked-crooked.com/a/d593674a60b92cb3 – P.W Sep 10 '18 at 09:19
Yes, that's why I printed inside the for loop. If you want to preserve the values in Register, then you have to make Register a 2 dimensional array and copy the return value of ToUpper into it. – P.W Sep 10 '18 at 09:26
Thank you sir that's the example i ask for.Can you give me an example with this? – Dhi Sep 10 '18 at 09:30
Also it's better choice to use char array instead of pointers? – Dhi Sep 10 '18 at 09:36
Does the latest example I have provided satisfy you? – P.W Sep 10 '18 at 10:09
You are welcome. I will update the answer to include this solution also. – P.W Sep 10 '18 at 10:22

Muperman · Answer 2 · 2018-09-10T08:48:39.587

Your problem is, that you return txt.

Txt is only a local char pointer, that exists only inside the function ToUpper.

With

Register[i]=ToUpper(p);

you assign that pointer to Register[i].

Now you call ToUpper again and the old txt will be overwritten with the next characters TEST2.

Now Register[0] points to the start of the text TEST2.

This goes on until the last call with test4.

Then Register[0..3] all point to the same location and this will be the start of the last txt-characteres (TEST4). So you print TEST4 4 times.

(Big problem is, that the memory of txt isn't allocated anymore and can change anytime during runtime)

This works in your case but isn't very good code, because it uses the fixed size of your test-strings. But you can make it clean yourself afterwards.

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

char *Register[5];

char *ToUpper(char *const string)
{
    int i=0;

    for(i = 0; i<=strlen(string); i++)
    {
        if(( string[i]>='a')&&( string[i]<='z'))
            string[i]=string[i] - 32;   
    }

    return string;
}

int main () {
    int i =0;
    for(i=0;i<=4;i++)
    {
        Register[i]=(char*)malloc(strlen("testN"));
        if(i==0)strcpy(Register[i],"test1");
        if(i==1)strcpy(Register[i],"test2");
        if(i==2)strcpy(Register[i],"test3");
        if(i==3)strcpy(Register[i],"test4");
        ToUpper(Register[i]);
    }
    printf("%s",Register[0]);
    printf("%s",Register[1]);
    printf("%s",Register[2]);
    printf("%s",Register[3]);

   return(0);
}

note that the code works only because the fixed sizes of your examples. Since i don't know what you want to do, pls change the fixed malloc length the way you need it.

So how can i allocate txt memory? or how can change my code for having different name in each my registers — Dhi, Sep 10 '18 at 08:25
It doesnt print anything... Also in upper case do i need string or *string? — Dhi, Sep 10 '18 at 08:54
just copy the whole code and past it here https://coliru.stacked-crooked.com/, then hit "Compile Link run ..." it works — Muperman, Sep 10 '18 at 08:59

Creating a custom uppercase method

2 Answers2