Q&A
このままではコードが読みづらいので、質問を編集し、<code>ボタンを押し、出てくる’’’の枠の中にコードを貼り付けてください
文字配列 s に格納された文字列の 2 倍の長さの文字列をちょうど格納できるサイズの文字配列を動的に割り当て, そこに s 中の文字列を 2 回繰り返したものをコピーし, その文字配列の先頭要素へのポインタを返す関数を作成
ただし、strcpy と strcat を用いてはならない.
（例）
元の文字列 ： abc
書き換えた文字列 ： abcabc
下記が今かけているプログラムなのですが、
これだと文字列はそのまま表示されてしまいます。
改善点を教えていただけないでしょうか？
c言語
1#include<stdio.h> 2#include<stdlib.h> 3#include<string.h> 4 5char *strdouble(char s[]){ 6 7 char *d = NULL; 8 int i; 9 10 //sの長さ*2の文字配列を割り当てる 11 d = (char *)malloc(sizeof(char)*(strlen(s) + strlen(s))); 12 13 for(i = 0; s[i]!= '\0'; i++){ 14 15 d[i] = s[i]; 16 } 17 18 d[i] = '\0'; 19 20 return d; 21} 22
ちなみにmainプログラムは下記になります。
mainプログラムの修正はしてはいけないです。
c言語
1#include <stdio.h> 2#include <stdlib.h> 3#define SLEN 126 4#define SFMT "%126s" 5 6char *strdouble(char s[]); 7 8int main(void) 9{ 10 char s[SLEN+1]; 11 char *s2 = NULL; 12 13 for (;;) { 14 fprintf(stderr, "s = "); 15 if (scanf(SFMT,s)==EOF) { break; } 16 s2 = strdouble(s); 17 printf("%s\n",s2); 18 free(s2); 19 s2 = NULL; 20 } 21 22 return 0; 23}
わかりました。
“strcpy と strcat を用いてはならない. ”最近こういう問題（標準関数を使うな的な）が多いんだけど？
できればscanf使うなぐらいしてほしいもんだけど。。
教える側が、scanf()以外知らないとか(；・∀・)
地味ですけど、文字列の取り扱いはプログラミングの基本的な制御構造を色々使うだろうから、学ぶ題材には良いと思います。K&R本でもそういうのが多かった気がしますし。
「strcpy と strcat を用いてはならない」これはきっとmemcpyを使えと言うことですね。わかります。
strcpy と strcatは安全じゃないのでmemcpy使うのは理にかなってますね！(きっとそんな事考えてない
遊ばないでくださいｗ
回答4件
0
C
1char *strdouble(const char *s) 2{ 3 char *d = malloc(strlen(s) * 2 + 1); 4 if (d) sprintf(d, "%s%s", s, s); 5 return d; 6}
投稿2019/11/17 04:32
総合スコア8224
raccy さんへのコメントは見ませんでしたか?
まずエラーにならない、としても NULL を返すのは、いかがなものか？
0
先の回答者であるLouiS0616さんのご指摘に加えて、目には見えていませんが、大事なことをひとつ忘れています。
C
1d = (char *)malloc(sizeof(char)*(strlen(s) + strlen(s)));
の部分で、末端に\0文字を入れる分が足りていません。
（誤認により削除）更に細かい点を指摘すると、 ですので、sizeof(char)*(strlen(s)でchar型のバイト数を意識している割には、+ strlen(s)の方では考慮されていません。
C
1d = (char *)malloc(sizeof(char)*(strlen(s) + strlen(s)) + sizeof(char)); 2 3またはいっそ、 4 5d = (char *)malloc((strlen(s) * 2) + 1);
と言ったところでしょうか。
投稿2019/11/16 07:42
編集2019/11/16 08:02総合スコア9254
なるほど、ありがとうございます。
@dodox86 さん
> 更に細かい点を指摘すると、 ... の方では考慮されていません。
その点については、sizeof(char) * (strlen(s) + strlen(s)) と括弧が付いているので問題無いのでは？
＞ LouiS0616さん
。。。あっ！ まさに仰る通りでした。カッコの位置を完全に見誤っていました。ご指摘どうもありがとうございます。訂正します。
お恥ずかしい限りです。修正させていただきました。
大変失礼いたしました。＞hikaru_love_n さん
いえいえ、こちらこそあまりわかっていなかったので、、、
ありがとうございます！
0
ベストアンサー
C
for(i = 0; s[i]!= '\0'; i++){
d[i] = s[i];
}
一周分しかコピーしていないように見えます。
iの値を保持したまま、この処理を二回繰り返すように書き換えれば良いです。
またmallocを使う際は、領域が確保できているか確認するようにすると良いでしょう。
####返り値
関数 calloc() と malloc() は、割り当てられたメモリーへのポインターを返す。 割り当てられたメモリーは、あらゆる組み込み型に対応できるようにアラインメントされる。 エラーの場合、これらの関数は NULL を返す。
引用元： Man page of MALLOC
コードの貼り方について
teratailには、コードを見やすく表示する機能があります。
質問編集画面を開き、コードを選択した状態で<code>ボタンを押して下さい。
投稿2019/11/16 07:23
編集2019/11/16 07:28総合スコア35668
iの値を保持したまま二回繰り返すように書くのはどのようにしたらいいですか？もう一回forループを書くわけではないですよね？
codeの貼り方ありがとうございます。
もう一回forループを書いても良いですし、forループ自体をfor文でぶん回しても良いです。
i = 0 を書く場所は意識しないと期待通りには動作しないでしょう。
※勘違いだったため削除※
i = 0をfor文の2つ目に書くとうまくいかないのは分かるのですが、
どう二回目を表現したらいいかなかなかわかりません、、、
編集できていないですか？
> i = 0をfor文の2つ目に書くとうまくいかないのは分かるのですが、
どう二回目を表現したらいいかなかなかわかりません、、、
次のような処理をfor文で表現できれば良いわけです。
d[0] = s[0];
d[1] = s[1];
d[2] = s[2];
d[3] = s[0];
d[4] = s[1];
d[5] = s[2];
案1: iとは別のカウンタjを用意し、sの添え字に使う
案2: sの添え字を i % (sの文字列長) にする
前者の方が簡単です。
> 編集できていないですか？
すみません、私の思い違いだったようです。編集ありがとうございます。
jが入力された文字列文の長さまでいけば、最初に戻るとしたらいいのですかね？
for文を二回書くのであれば、両方の初期化部で j = 0 としてしまえば良いです。
二重のfor文を書くのであれば、内部のループの初期化部で j = 0 とすれば良いでしょう。
1つ目のfor文はfor(i = 0; s[i]!= '\0'; i++)
で大丈夫ですか？
その場合2つ目のfor文の定義はどう考えればいいかなと、、、
for文は二重に書こうと思っています
内側のfor文は文字列を一度コピーするためのループです。
外側のfor文は、内側のfor文を複数回繰り返すためのループです。
ですので外側のfor文については、for(int k = 0; k < 2; k++) とでも書けば良いです。
---
i, j, k と三つも出てきてややこしくなってしまったので、より適切な名前を付け直した方が良いかもしれません。(私が持ち出した変数名なのに、すみません)
出力がabcとは限らないので文字列の長さにしたい場合はk<strlen(s)で大丈夫ですか？
外側のfor文がkで内側がiでカウントがjですよね？
> 外側のfor文がkで内側がiでカウントがjですよね？
だいぶややこしくなってきたので、名前を変えます。
・ dの添え字に使う idx_d 。 0 1 2 3 4 5 6 7 と変化させたい。
・ sの添え字に使い idx_s 。 0 1 2 0 1 2 と変化させたい。
・ コピー回数のカウンタとなる i 。(旧iとは別)
外側のループのカウンタは i です。
内側のループはカウンタのように idx_s を扱います。
> 出力がabcとは限らないので文字列の長さにしたい場合はk<strlen(s)で大丈夫ですか？
内側の条件部は idx_s < strlen(s) となるでしょうね。
これは実は若干効率が悪い書き方なんですが、そこは練習と言うことで無視します。
for(i = 0; s[i]!= '\0'; i++){
for(idx_s = 0; idx_s= strlen(s); idx_s++){
d[i] = s[i];
}
}
現在このように書いているのですが、
このままで実行すると動作が固まってしまいます。
内側のループの条件部が変ではありませんか。
また肝心の d[i] = s[i] の部分が修正されていないです。
0からではないのでしょうか？
呼び方はいろいろあるかもですが、ここでは for(初期化部; 条件部; 更新部) という意味で言っています。
=ではないですね、すいません。
＜=ですよね？
いえ、< ですかね。
例えば3文字だと s[0], s[1], s[2] の三要素が有効ですので。
for(i = 0; s[i]!= '\0'; i++){
for(j = 0; j < strlen(s); j++){
d[i] = s[j];
}
}
こう書いたところ、abcはcccに、1234567890は000000000という表示結果になりました。違うところご指摘お願いします。
いいところまで来たじゃないか。
内側のループは、元の文字列を一回コピーする、のだから、
外側のループを使って２回繰り返すように修正したら、完成に近づくのでは？
内側のループで i++; しないと、一回のコピーもできないか。
念の為
> 外側のループを使って２回繰り返すように修正したら…
２回繰り返すためのループだから、外側のループで文字列の終端（'\0'）をチェックする必要は無いんですよ。
2回繰り返すという表現の仕方がわかりません。どういうふうに書いたら2回繰り返すになるのかがわかりません、、、
Ａという処理をＮ回繰り返す、というパターンはこうです。
for (k = 0; k < N; k++) {
A;
}
このパターン、教わりませんでしたか？
原則は、使っている変数 k の値を処理Ａの中で変更しないことです。
for(i = 0; i < 2; i++){
for(j = 0; j < strlen(s); j++, i++){
d[i] = s[j];
}
}
と書き換えたところ、abcはabc、1234567890は123456789-とそのまま表示になったのですが、、、どうすればいいですか？？
すいません、1234567890は、1234567890-となりました。
変数 k の値を処理Ａの中で変更しない、と書いたんだけどな。
２回繰り返すために i を使い、文字列をコピーするにも i を使ったら、そりゃーヘンな結果になりますよ。
繰り返しは変数 k を使いなさい。
2回繰り返しに k を使えば、こうなります。
for(k = 0; k < 2; ki++){
for(j = 0; j < strlen(s); j++, i++){
d[i] = s[j];
}
}
うまく動作しないでしょう。でも信用してほしい。あと、どこかで、ひとつだけ、処理を書けば完成する。
使う変数の役割を整理しよう。
i は d[i] で使う、0, 1, 2, ... どこまで行くのかな？
j は s[j] で使う、0, ... , strlen(s) になったら内側のループを終了する
k は２回繰り返しに使う、0, 1 と変化し、２で終了する
今、何が足りないだろうか？
iはstrlen(s)の長さの二倍分までいきますよね？
その表示が足りないのでしょうか？
足りないのは i に関する事だけど、上限じゃないのです。だとしたら、なんだろう？
初期化？？
@hikaru_love_n さん
はい、iの値は適切なタイミングで0になる必要がありますね。
次の A, B, C のうちどこでしょう？
A
for(k = 0; k < 2; k++){
B
for(j = 0; j < strlen(s); j++, i++){
C
d[i] = s[j];
}
}
Bだと思ってやっていたのですが違うのでしょうか？
sの文字列が3のとき、Bにi=0を置くと次のように処理が進むことになります。
d[0] = s[0];
d[1] = s[1];
d[2] = s[2];
d[0] = s[0];
d[1] = s[1];
d[2] = s[2];
これで本当に正しいでしょうか？
よく考えたらAですね！
Aでやってみたらうまくできました。
ありがとうございます。
解決したようで何よりです。
@rubato6809 さん
有用なコメントを多く寄せて下さり、ありがとうございます。
他の人だったらどんなアドバイスをするのかなぁと少し興味があったもので、静観させて貰っていました。自己本位な行動ですみません。
呼びかけを多く用い、コミュニケーションを促しているのが印象的でした。参考にさせていただきます。
よかったです。私もホッとしました。
d[0] = s[0];
d[1] = s[1];
d[2] = s[2];
d[3] = s[0];
d[4] = s[1];
d[5] = s[2];
私は、これを早い段階で示された事に感心しました。こんな風に動きを具体化して見せることが初心者には大事だと思います。
ループって、理解するまでは動作のイメージが全然湧かないですもんね。
目の前に相手がいれば指で実行順序を示せるわけですが、画面越しでそれが難しい以上できるだけ具体的なイメージを伝えようという意識はあります。
0
考えられるだけの改善点をあげてみました。
C
1// #includeと<の間はスペースを入れる 2#include <stdio.h> 3#include <stdlib.h> 4#include <string.h> 5#include <stdint.h> 6 7// assert用 8// テスト時はこのままにし、本番ではコメントアウトを外す 9// #defien NDEBUG 10#include <assert.h> 11 12// 本来はヘッダ部分を分離して、それ読み込むが、 13// 今回はmainの方に直接書いてあるため、個々に書く。 14// mainと同じファイルに書く場合はこの部分は不要 15#define SLEN 126 16 17/** 18 * 受け取った文字列を二回表示にして返す。 19 * 引数は`SLEN`文字以下のnull終端文字列で無ければならない。 20 * そうで無い場合の動作は未定義とする。(何が起こるかは不明) 21 * この関数呼び出し時は、プログラム実行時は十分なメモリが存在し、 22 * malloc等の処理が失敗しない状態で無ければならない。 23 * メモリが不十分で、malloc等が失敗する場合の動作は未定義とする。 24 * これらの条件をつけた経緯はrubato6809さんのコメントを参考にすること。 25 */ 26 27// ループ変数を除き一文字のみの変数名や引き数名は避ける。 28char *strdouble(char str[]) { // ) {の間はスペースを入れる。 29 // ローカル変数は必要になったときに宣言する。 30 // 関数の先頭でまとめて宣言しない。 31 32 // strlen()は重い処理のため、呼び出しは一回のみにするように変数に入れる。 33 // strlen()の戻り値はsize_t型。 34 size_t len = strlen(str); 35 36 // 確保するメモリはlenの2倍に1足した数だが、その値がSIZE_MAXを越える場合、 37 // その計算がオーバーフロー起こす可能性がある。 38 // 受け取る引数の文字列の長さに制限が無い場合は、 39 // あらかじめSIZE_MAXを越えないかを確認しておく 40 // しかし今回は、SLENによってサイズの最大サイズが固定であるため、 41 // SLENによって条件を満たさない場合があるかの確認をテスト時のみ行う。 42 // NDEBUGが有効の場合は下記はコンパイル時に除外される。 43 assert(SERN <= (SIZE_MAX - 1) / 2) 44 45 // strの長さ(len)の2倍に1足した大きさの文字配列を割り当てる 46 // null終端文字列は終端null文字分のために、文字列の長さに+1だけ大きさが必要。 47 // 変数名はわかりやすい名前を付ける。 48 // 宣言と同時に代入する。関数の最初で宣言はしない。 49 // sizeof(char)は例外なく必ず1である。charの場合は書かない。 50 char *double_str = (char *)malloc(len * 2 + 1); 51 52 // メモリが十分に確保されていることが保証できなければ、 53 // mallocしたら必ず戻り値がNULLでないかを確認する必要がある。 54 // しかし、rubato6809さん曰く「まずエラーになりません。」となっている。 55 // SLENのサイズが小さくても、空きメモリが極めて少ない状態でプログラムを 56 // 実行した等の特殊な状況においては、失敗する恐れがあるが、 57 // 今回は、そのような場合は動作未定義として、チェックはしないこととする。 58 59 // for文等でちまちまと代入せずにmemcpy()を使う。 60 // 実装にもよるがmemcpy()はある程度のバイト列ごとにまとめてコピーするため、 61 // 長い文字列では、1文字ずつコピーするよりも数倍の速度差が出る可能性がある。 62 // destとsrcがオーバーラップすることは無いため、memmove()にする必要は無い。 63 memcpy(double_str, str, len); 64 65 // lenだけズレた位置にもう一度コピーする。 66 // 終端null文字も含めるために、lenに+1した分をコピーする。 67 memcpy(double_str + len, str, len + 1); 68 69 // あとはdouble_strを返すだけ、お疲れ様でした。 70 return double_str; 71}
投稿2019/11/16 10:19
編集2019/11/17 06:03総合スコア21737
回答内容もさることながら、return直前の「お疲れ様でした」のコメントにプログラマーと関数への誠意（？）を感じました。高評価させていただきました。
return NULL; が２箇所ありますが、これだと main() の中の
printf("%s\n", s2); が異常終了するでしょう。
> rubato6809さん
サイズ超える場合やメモリ確保できないときにどうすれば良いのか私にはわかりませんでした。rubato6809さんが考えるより良い方法を教えていただけませんか？その方法で書き換えたいと思います。
> raccyさん
そうしたエラー処理を考えなくても良い、が私の第一の答えです。
根拠は、出題者に確認したから、では勿論なく、私がそう感じるから。今の質問者の段階で求められているのは、そのようなエラー処理ではない、或いは重視されていないと思うから。
少なくとも if (len > (SIZE_MAX - 1) / 2) return NULL; は不要でしょう。なぜなら scanf(SFMT,s)==EOF は scanf("%126",s)==EOF ですからstrdouble() に渡される文字列は長くても 126 文字程度に制限されていると読み取れます。さらにmalloc() も、まずエラーになりません。
でも一般論として、NULLを返せない条件でエラー対策するとしたら、今思いつくのは4つ。
1. 引数の文字列をそのまま return str; で返す（ダブルにならないからエラーとわかる）
2. return "ERROR: can't allocate memory."; のようにエラーを示す文字列を返す
3. errno のような、別の手段を使う
4. main() 関数を書き換えてもらう（笑）
これらが第二の答えです。
ついでに言えば。
じきに memcpy() は必要になるだろうから、今のうちから伝えておくのは構わないと思いますが、それを伝えるのは二重ループが分かった後でも良い。
おそらく今の段階で質問者に求められているのは、memcpy() よりも、「for文等でちまちまと代入」することだと思います。現に質問者は、それを組み立てられずにいる。
この難関？を理解できずに、この先使い物になるだろうか？memcpy() のありがたみを正しく理解できるだろうか？
私が出題者なら、まずはちまちまループで回す経験をしてもらいたい。
今の質問者は思考に足枷が嵌まっている状態ではないか、という気がします。その場合は単純に答え（質問者が求めている二重ループの回答）を示すのもひとつの手です。なんだ、こうすれば良かったのか、と思い知ることが自分自身の足枷を外すきっかけに十分なるからです。
> rubato6809さん
コメントを参考に、エラー処理について、ドキュメントに条件を見たなさない文字列が来た場合や空きメモリが少ない場合は動作未定義であると明記して、エラー処理を行わないようにしました。
一般論という対策もしようと思ったのですが、
1. そのまたstrを返した場合、free(s2);の動作が未定義。
2. 文字列リテラルを返した場合、同じくfree(s2);の動作が未定義。
3. 別手段がわかりませんでした。strerror()の結果を返してもfree(s2)で動作未定義です。
4. これができるなら、これが一番いいと思いますが。
となり、「一般論」としているせいだと思いますが、rubato6809さんの一般論では対応できませんでした。
わたしは天邪鬼な人間なので「出題者の意図など糞食らえ」です。いかに、memcpy()等を使うなと言うならmemcpy()もmemmove()等も使うなと問題に書いておくべきです。問題文とはありのままであるべきで、その出題者の意図にそぐわないからダメというなら、不備がある問題文を見るべきです。
色んな所で言っていますが、私は「質問者」を軽視しています。見ているのは「質問」だけです。凄く冷たい人間です。どういう意図があろうが、「質問」にある内容に対して回答するだけです。今回の質問に「改善点」とあったので、私は考えるだけの改善点を示しただけに過ぎません。
そういう態度が気に入らないのであれば、それで結構です。
> raccyさん
free(s2);の動作が未定義・・・確かに。それを考慮すると、この場合 assert() はベストな感じがしますね。assert() は今後も折りにふれ使うものだし、関数の戻り値を検査するのは原則的な態度であって、早い段階から意識づけすべきですから。低評価は外します。
ただ、それなら malloc() の戻り値も assert() してみせたらよかったのに、と思います。
なお、私的には main() 関数が s2 の宣言時に NULL を初期値にして、ループする都度 s2 = NULL; するのが気に入らない。この点、出題者の意図を問いただしたい気分です笑。
assert()は本番時には外すモノだからmalloc()のような実行時エラー検出に付けるのはどうなんだろうと思って付けませんでした。テスト時に意図しない範囲の定数が設定されていないかとか、境界値テストで意図せぬ範囲以上になってないか確認するとかなら有用なんですけど、あまり使われてないようで悲しいです。
あなたの回答
tips
プレビュー
