-1

I attempted to make my own mystrcpy() function, which takes the same arguments as the standard function. It is not responding. The array does not get copied.

size_t Mystrlen(const char* s)
{
    int i = 0;
    while (s[i] != '\0')
    {
        i++;
    }
    return i;
}

char* Mystrcpy(char* s1, const char* s2)
{
    for (int i = 0; i < Mystrlen(s2); i++)
        s1[i] = s2[i];
    return s1;
}

int main()
{
    char s1[50];
    char s2[50];
    cout << "enter the value of second string\n";
    cin >> s2;
    Mystrcpy(s1, s2);
}

https://godbolt.org/z/zWxqxn3Kx

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
malscode
  • 31
  • 5
  • We would need to see a function named `Mystrlen` to discuss or debug what this code does. The code should be a [mre]. – Drew Dormann Mar 30 '22 at 16:21
  • I am so sorry for the inconvenience. I have added the function. – malscode Mar 30 '22 at 16:25
  • 3
    now, define "not working" You call this function in the posted code, but do *nothing* with the resultant `s1` – WhozCraig Mar 30 '22 at 16:25
  • _"The array does not get copied"_ I assure you that in the code shown here, the array _does_ get copied. Up to but not including the null-terminator. A link has been added to your question, showing the copied characters. – Drew Dormann Mar 30 '22 at 16:28
  • You should assign the result of `Mystrlen(s2)` into a constant temporary variable, then use the variable in the loop. The string `s2` doesn't change inside the loop, so there is no need to call the length function more than once. – Thomas Matthews Mar 30 '22 at 16:39
  • 1
    `Mystrcpy` doesn't need to call `Mystrlen`. Just copy characters until you hit the nul terminator. And don't forget to copy the terminator! Typical hacker-head code for doing this is `while (*s1++ = *s2++) ;`. If that doesn't make sense to you, don't worry about it. – Pete Becker Mar 30 '22 at 17:19

1 Answers1

7
  1. You'd better call Mystrlen once before the loop, instead of each iteration (to check the loop condition). It's very inefficient this way.
  2. You didn't copy the zero termination to the destination string.
  3. Not related to your bug, but anyway it is better to avoid using namespace std - see here Why is "using namespace std;" considered bad practice?
  4. As @Pete Becker commented, MyStrcpy doesn't need to call Mystrlen at all - see above. I kept it the way it was assuming it is done like this for learning purposes.
  5. In real production code, the length of the output char array (s1) must be validated, otherwise Mystrcpy can cause a buffer overrun (if s1 is allocated number of chars less than required for the content of s2).

Fixed code (up to the notes above):

#include <iostream>

// size_t Mystrlen(const char* s) ...   // Same as in your code

char* Mystrcpy(char* s1, const char* s2)
{
    size_t len = Mystrlen(s2);  // Call once before the loop
    for (int i = 0; i < len; i++)
    {
        s1[i] = s2[i];
    }
    s1[len] = '\0';     // Add zero termination
    return s1;
}

int main()
{
    char s1[50];
    char s2[50];
    std::cout << "enter the value of second string\n";
    std::cin >> s2;
    Mystrcpy(s1, s2);
    std::cout << "copied string: " << s1 << std::endl;
}
wohlstad
  • 12,661
  • 10
  • 26
  • 39
  • 1
    @malscode you can consider to accept my answer if it was useful (click the '✔' below the score). You can also consider to up-vote (https://stackoverflow.com/help/why-vote ). – wohlstad Jun 16 '22 at 12:58