K& R演習:私のコードは機能しますが、臭いです;クリーンアップのアドバイス?
-
03-07-2019 - |
質問
私はK& Rの本に取り組んでいます。私は演習を行ったよりもはるかに先を読みましたが、ほとんど時間が不足しています。私は追いつき、チュートリアルである第1章のほぼすべての演習を完了しました。
私の問題は演習1-18です。演習は次のとおりです。
末尾の空白を削除するプログラムを作成し、 入力行のタブ、および完全に空白の行を削除する
私のコード(下)はそれを行い、動作します。私の問題は、私が実装したトリム方法です。それは...間違っている...何とか感じます。コードレビューでC#で同様のコードを見た場合のように、私はおそらく夢中になります。 (C#は私の専門分野の1つです。)
誰でもこれをきれいにするためのアドバイスを提供できますか?-アドバイスはK& R.(完全なCライブラリを使用してこれをクリーンアップする無数の方法があることを知っています。ここでは、第1章と基本stdio.hについて説明しているだけです。) (私は、結局のところ、学ぼうとしています!そして、ここの専門家よりも誰から学ぶべきですか?)
#include <stdio.h>
#define MAXLINE 1000
int getline(char line[], int max);
void trim(char line[], char ret[]);
int main()
{
char line[MAXLINE];
char out[MAXLINE];
int length;
while ((length = getline(line, MAXLINE)) > 0)
{
trim(line, out);
printf("%s", out);
}
return 0;
}
int getline(char line[], int max)
{
int c, i;
for (i = 0; i < max - 1 && (c = getchar()) != EOF && c != '\n'; ++i)
line[i] = c;
if (c == '\n')
{
line[i] = c;
++i;
}
line[i] = '\0';
return i;
}
void trim(char line[], char ret[])
{
int i = 0;
while ((ret[i] = line[i]) != '\0')
++i;
if (i == 1)
{
// Special case to remove entirely blank line
ret[0] = '\0';
return;
}
for ( ; i >= 0; --i)
{
if (ret[i] == ' ' || ret[i] == '\t')
ret[i] = '\0';
else if (ret[i] != '\0' && ret[i] != '\r' && ret[i] != '\n')
break;
}
for (i = 0; i < MAXLINE; ++i)
{
if (ret[i] == '\n')
{
break;
}
else if (ret[i] == '\0')
{
ret[i] = '\n';
ret[i + 1] = '\0';
break;
}
}
}
編集:ここに表示されているすべての役立つヒントに感謝します。私はまだCのn00bであり、具体的にはまだポインターに達していないことを人々に思い出させたいと思います。 (K&amp; RのCh.1に関するビットを思い出してください-Ch.1はポインターを実行しません。)私は「ちょっと」それらのソリューションのいくつかを入手しますが、それらは私がいる場所のためにまだ進んでいます...
そして私が探しているもののほとんどは、トリムメソッド自体です。具体的には、 3 回ループしているという事実です(これは非常に汚い感じです)。 (Cの高度な知識がなくても)もっと賢く触っただけなら、これはもっとすっきりしていたかもしれません。
解決
2つのバッファーを用意する理由はありません。入力行をその場でトリミングできます
int trim(char line[])
{
int len = 0;
for (len = 0; line[len] != 0; ++len)
;
while (len > 0 &&
line[len-1] == ' ' && line[len-1] == '\t' && line[len-1] == '\n')
line[--len] = 0;
return len;
}
行の長さを返すことにより、長さがゼロでない行をテストすることで空白行を削除できます
if (trim(line) != 0)
printf("%s\n", line);
編集:ASCIIエンコードを想定して、whileループをさらに簡単にすることができます。
while (len > 0 && line[len-1] <= ' ')
line[--len] = 0;
他のヒント
第1章に固執している場合、それは私にはかなり良さそうです。コードレビューの観点からお勧めするものは次のとおりです。
Cで等価性をチェックするときは、常に定数を最初に置きます
if (1 == myvar)
これにより、誤って次のようなことをすることはありません:
if (myvar = 1)
C#でそれを回避することはできませんが、Cで正常にコンパイルされ、デバッグする本当の悪魔になる可能性があります。
trim()は大きすぎます。
必要だと思うのは、strlen-ish関数です(先に進んで、int stringlength(const char * s)と書いてください)。
その後、int scanback(const char * s、const char * matches、int start)と呼ばれる関数が必要になります。一致が見つかった最後のインデックス。
その後、int scanfront(const char * s、const char * matches)という関数が必要ですが見つかりました。
次に、int charinstring(char c、const char * s)と呼ばれる関数が必要です。この関数は、cがsに含まれる場合は0以外を返し、そうでない場合は0を返します。
これらに関してトリムを書くことができるはずです。
while構文の個人用:
私は次を好む:
while( (ret[i] = line[i]) )
i++;
to:
while ((ret[i] = line[i]) != '\0')
++i;
両方とも!= 0に対してチェックしますが、最初のほうが少しきれいに見えます。 charが0以外の場合、ループ本体が実行され、そうでない場合はループから抜け出します。
「for」ステートメントについても、構文的には有効ですが、次のことがわかります。
for ( ; i >= 0; --i)
私には「奇妙」に見えますが、実際には潜在的なバグに対する潜在的な悪夢のような解決策です。このコードをレビューしている場合、赤く光る警告のようになります。通常、既知の回数を反復するためにforループを使用し、そうでない場合はwhileループを使用します。 (いつものように、ルールには例外がありますが、これは一般的に当てはまることがわかりました)。上記のforステートメントは次のようになります。
while (i)
{
if (ret[i] == ' ' || ret[i] == '\t')
{
ret[i--] = '\0';
}
else if (ret[i] != '\0' && ret[i] != '\r' && ret[i] != '\n')
{
break;
}
}
まず第一に:
int main(void)
main()のパラメーターを知っています。彼らは何もありません。 (またはargc&amp; argvですが、第1章の資料ではないと思います。)
スタイルに関しては、K&amp; Rスタイルのブラケットを試してください。垂直方向のスペースでははるかに簡単です。
void trim(char line[], char ret[])
{
int i = 0;
while ((ret[i] = line[i]) != '\0')
++i;
if (i == 1) { // Special case to remove entirely blank line
ret[0] = '\0';
return;
}
for (; i>=0; --i) { //continue backwards from the end of the line
if ((ret[i] == ' ') || (ret[i] == '\t')) //remove trailing whitespace
ret[i] = '\0';
else if ((ret[i] != '\0') && (ret[i] != '\r') && (ret[i] != '\n')) //...until we hit a word character
break;
}
for (i=0; i<MAXLINE-1; ++i) { //-1 because we might need to add a character to the line
if (ret[i] == '\n') //break on newline
break;
if (ret[i] == '\0') { //line doesn't have a \n -- add it
ret[i] = '\n';
ret[i+1] = '\0';
break;
}
}
}
(コメントも追加し、1つのバグを修正しました。)
大きな問題は、MAXLINE定数の使用です。main()は、 line および out 変数にのみ使用します。それらに対してのみ機能しているtrim()は、定数を使用する必要はありません。 getline()で行ったように、サイズをパラメータとして渡す必要があります。
個人的には次のようなコードを入れます:
ret[i] != '\0' && ret[i] != '\r' && ret[i] != '\n'
別の関数(または定義マクロ)に
- trimは実際に1つのバッファのみを使用する必要があります(@Ferruccioが言うように)。
- @plinthが言うように、トリムを分割する必要があります
- trimは値を返す必要はありません(空の文字列を確認する場合は、テスト行[0] == 0)
- 追加のCフレーバーには、インデックスではなくポインターを使用します
-行末に移動(0で終了。 -行の先頭ではなく、現在の文字がスペースの場合は、0に置き換えます。 -1文字を削除
char *findEndOfString(char *string) {
while (*string) ++string;
return string; // string is now pointing to the terminating 0
}
void trim(char *line) {
char *end = findEndOfString(line);
// note that we start at the first real character, not at terminating 0
for (end = end-1; end >= line; end--) {
if (isWhitespace(*end)) *end = 0;
else return;
}
}
同じことを行う別の例。 C99固有のものを使用することにより、若干の違反がありました。 K&amp; Rにはありません。また、starndardライブラリの一部であるassert()関数も使用しましたが、おそらくK&amp; Rの第1章ではカバーされていません。
#include <stdbool.h> /* needed when using bool, false and true. C99 specific. */
#include <assert.h> /* needed for calling assert() */
typedef enum {
TAB = '\t',
BLANK = ' '
} WhiteSpace_e;
typedef enum {
ENDOFLINE = '\n',
ENDOFSTRING = '\0'
} EndofLine_e;
bool isWhiteSpace(
char character
) {
if ( (BLANK == character) || (TAB == character ) ) {
return true;
} else {
return false;
}
}
bool isEndOfLine(
char character
) {
if ( (ENDOFLINE == character) || (ENDOFSTRING == character ) ) {
return true;
} else {
return false;
}
}
/* remove blanks and tabs (i.e. whitespace) from line-string */
void removeWhiteSpace(
char string[]
) {
int i;
int indexOutput;
/* copy all non-whitespace character in sequential order from the first to the last.
whitespace characters are not copied */
i = 0;
indexOutput = 0;
while ( false == isEndOfLine( string[i] ) ) {
if ( false == isWhiteSpace( string[i] ) ) {
assert ( indexOutput <= i );
string[ indexOutput ] = string[ i ];
indexOutput++;
}
i++; /* proceed to next character in the input string */
}
assert( isEndOfLine( string[ i ] ) );
string[ indexOutput ] = ENDOFSTRING;
}
第1章または「K&amp; R.ポインターを想定していますか
#include "stdio.h"
size_t StrLen(const char* s)
{
// this will crash if you pass NULL
size_t l = 0;
const char* p = s;
while(*p)
{
l++;
++p;
}
return l;
}
const char* Trim(char* s)
{
size_t l = StrLen(s);
if(l < 1)
return 0;
char* end = s + l -1;
while(s < end && (*end == ' ' || *end == '\t'))
{
*end = 0;
--end;
}
return s;
}
int Getline(char* out, size_t max)
{
size_t l = 0;
char c;
while(c = getchar())
{
++l;
if(c == EOF) return 0;
if(c == '\n') break;
if(l < max-1)
{
out[l-1] = c;
out[l] = 0;
}
}
return l;
}
#define MAXLINE 1024
int main (int argc, char * const argv[])
{
char line[MAXLINE];
while (Getline(line, MAXLINE) > 0)
{
const char* trimmed = Trim(line);
if(trimmed)
printf("|%s|\n", trimmed);
line[0] = 0;
}
return 0;
}