Что не так в моей функции конкатенации mystrcat (char *, char *, char *)?

Question

Недавно я дал интервью и попросил написать mystrcat(*s1, *s2, *s3), где s1 и s2 - исходная строка, а объединенные результаты даны s3 Мне сказали, не беспокойтесь о выделении памяти s3 и предположите, что s1 и s2 не являются пустыми / недействительными строками. Поэтому я написал следующую хромую (сырую) программу. Мне сказали, что что-то не так с s3 или что-то может пойти не так с s3. Не могли бы вы сказать, что это такое / это может быть?

void mystrcat(char *s1, char *s2, char *s3)
{
    if (! (s1 || s2 || s3)) return; // one/more pointers are invalid

    // copy string s1 into s3
    while(*s1) {
      *s3 = *s1;
      s1++;
      s3++;
    }

    // concatenate string s2 into s3
    while(*s2) {
      *s3 = *s2;
      s2++;
      s3++;
    }

    *s3 = '\0';
}

Не могли бы вы сказать мне, что здесь не так? Что может быть более профессиональным способом сделать это?

Answer 1

Два возможных пункта

Во-первых, вам сказали, что входные данные и результат указали на допустимые строки, поэтому проверка на достоверность, возможно, не нужна. И если бы это было необходимо, вы бы с треском провалились. Лучше было бы:

 void mystrcat(char *s1, char *s2, char *s3)
    {
        ASSERT( s1 );
        ASSERT( s2 );
        ASSERT( s3 );
        ....

Тогда вы в основном написали strcat / strcpy, когда могли бы использовать их повторно:

void mystrcat(char *s1, char *s2, char *s3)
{
    strcpy( s3, s1 );
    strcat( s3, s2 );
}

И если бы я брал у вас интервью для чего-то другого, кроме поста для юниоров, я бы ожидал, что вы укажете мне, что указанный интерфейс mystrcat разработан очень тщательно и подробно описывает, как вы могли бы его улучшить.

Answer 2

if (! (s1 || s2 || s3) return; // one/more pointers are invalid

Должно быть

if ((!s1) || (!s2) || (!s3)) return;

Answer 3

Вот мои комментарии

• Для s1 и s2 следует ввести const char*, поскольку вы не собираетесь их изменять.
• Вы не убедились, что s3 имеет достаточно выделенного пространства для хранения общей длины s1 и s2
• Вы молча терпите неудачу, когда пользователь передал неверные данные (значения NULL). Это не хорошо, потому что вызывающий абонент не может различить успешный и неудачный вызов
• Ваш метод проверки s1, s2 и s3 не равен нулю неверен (у Игоря все правильно)

Вопросы, которые мне хотелось бы, чтобы вы задали себе или ответили самостоятельно во время интервью

• Как мне выразить неудачу при копировании строки?
• Вам нужен точный клон strcat или более современный и лучше проверяющий аргументы?
• Могу ли я использовать другие стандартные функции str для завершения ответа?

Answer 4

В дополнение к предыдущим ответам, еще одна вещь, которая может пойти не так: s3 может указывать на середину строк s1 или s2. Если это законное условие, вам нужна более сложная реализация.

Answer 5

1. неверный чек

   if (! (S1 || s2 || s3)) return; // один или несколько указателей недействительны

   Он фактически проверяет, является ли хотя бы один указатель не равным 0, например по крайней мере один указатель действителен. Должно быть

   if (! S1 ||! S2 ||! S3) return;

2. Вы не можете проверить, большой ли s3 достаточно или если ты пишешь снаружи (но предположение было s3 большое достаточно верно? - но все равно это будет не работает в реальном мире)

3. Вы не можете пропустить ноль, который копируется с конца s1 в с3. Вы прикрепляете s2 после 0 в s3 так что в конечном итоге "s1stringvalueNULLs2stringvalue" (NULL здесь означает значение 0 или ноль или ноль в реальном коде, это для иллюстрации). Любой метод C, который ожидает, что нулевая завершенная строка будет смотрите только «s1stringvaluepart» и остановится на нуле.

Вот решение:

void mystrcat(char *s1, char *s2, char *s3)
{
    if (!s1 || !s2 || !s3) return;

    while (*s3++ = *s1++);
    if (!*(s3-1)) --s3;             // reverse over null in s1
    while (*s3++ = *s2++);

    *s3 = 0;
}

Вы можете проверить это с помощью:

#include <iostream>
using namespace std;

int main()
{
    char s1[] = "short";
    char s2[] = "longer";
    char s3[20];

    memset(s3, 0, sizeof(s3));
    mystrcat(0,s2,s3);
    cout << "2: " << s3 << endl;

    memset(s3, 0, sizeof(s3));
    mystrcat(s1,0,s3);
    cout << "3: " << s3 << endl;

    memset(s3, 0, sizeof(s3));
    mystrcat(s1,s2,0);
    cout << "4: " << s3 << endl;

    memset(s3, 0, sizeof(s3));
    mystrcat(s1,s2,s3);
    cout << "1: " << s3 << endl;

}

Answer 6

Есть несколько критических замечаний по поводу определения вашей функции, но в рамках условий постановки задачи ваша функция даст правильный ответ. Технически это не так.

Я предполагаю, что проблема не была эффективно передана, или критика интервьюера не была передана. Может быть, разъяснение было частью теста, а?

Вот краткое изложение возможных жалоб ...

• В этой строке есть логическая ошибка ...

  if (! (S1 || s2 || s3)) return;

... потому что он вернется, если все равны нулю, но вы, вероятно, хотите вернуть, если любой равен нулю Это не может привести к сбою, поскольку в заявлении о проблеме говорится, что ни один из них не может быть нулевым.

• Та же самая строка, упомянутая выше, молча завершается неудачей. Было бы лучше вернуть код ошибки, сгенерировать исключение или подтвердить. Тихий сбой - это плохая практика, но технически он корректен, потому что функция не может дать сбой, учитывая суть проблемы.
• s1 и s2 могут иметь тип const char * для лучшей читабельности и эффективности (const помогает оптимизировать некоторые компиляторы).
• Изменение переменных параметров часто считается плохим практика. Другое дело читаемость.
• Вы можете использовать существующие функции strcpy и strcat. Другое дело удобочитаемости и эффективности.

Хороший программист должен стремиться не только к технической корректности, но и к удобочитаемости, эффективности и надежной обработке ошибок, но в основном это субъективно и ситуативно. В любом случае, не обращая внимания на ошибку в вашем первом операторе if, я думаю, что ваша функция читается достаточно хорошо и выполняет свою работу. :)

Answer 7

Пожалуйста, поправьте меня, если я ошибаюсь. Не будет ли последний символ s3 '\ 0' после копирования s1? Таким образом, оставшиеся символы из s2 никогда не могут быть прочитаны в некоторых случаях?

Из моего понимания

while(*s3++ = *s1++);

будет копировать символы до тех пор, пока не будет достигнуто значение NULL, а '\ 0' не будет иметь значение NULL, или оно будет рассматриваться как таковое? Если я могу предположить, что s1 = "Hello" и s2 = "world!" тогда после копирования s1, s3 будет выглядеть так: Hello \ 0 и затем копировать "world!" в результате бит s3 будет выглядеть так: Hello \ 0 world! \ 0

Есть ли смысл? Правильно ли мое понимание?