このコードのどこが汚いか

Question

とても抽象的な質問で申し訳ないのですが、自分は独学のプログラミング初心者でc++のコードを今回初めて書きました。
ですが、とても汚い書き方をしているように思えてしまうので、改善点を教えていただきたいのです。

プログラムは、txtファイルから問題と答えを読み込んでコマンドプロンプトでクイズをするといったものです。
ans.txtとqus.txtにはそれぞれ問題と答えを行で分けて書いています。
今のところ文字化けなどはなく、自分の思った通りには動いています。

自分的には、変数の初期化がうるさいところや、for文の中身がぐちゃぐちゃなところが気に入らないです。
これが最善手であれば納得しますが、
もっといい書き方があればご教授いただきたいです...！

#include<iostream>
#include<fstream>
#include<string>
using namespace std;

string qus[200];
string ans[200];
string you;
int i = 0;



int main()
{
	// pemファイル読み込み
	ifstream qifs("qus.txt");
	string qline;

	if (qifs.fail()) {
		std::cerr << "File Open Error" << std::endl;
		return -1;
	}
	while (getline(qifs, qline)) {
		qus[i] = qline;
		i++;
	}

	i = 0;
	ifstream aifs("ans.txt");
	string aline;

	if (aifs.fail()) {
		std::cerr << "File Open Error" << std::endl;
		return -1;
	}
	while (getline(aifs, aline)) {
		ans[i] = aline;
		i++;
	}


	for (int i = 1; i < 200; i++)
	{
		cout << i << "." << qus[i - 1] << "\n";
		cin >> you;
		if (you == ans[i - 1])
		{
			cout << "〇\n";
		}
		else
		{
			cout << "×" << ans[i - 1] << "\n";
		}

	}
	return 0;
}

に勝利する
を改善する
を発達させる

.
.
.
以下200個

win
improve
develop

.
.
.
以下200個

Answer 1

using namespace std;

できればコレは main() 内に置く

string qus[200];
string ans[200];
string you;
int i = 0;

グローバル空間に置かない。

string qus[200];
string ans[200];

これだと200個で頭打ちになる。

...で、僕ならこう↓書く:

#include <iostream> // cout, endl
#include <fstream>  // ifstream
#include <string>   // string
#include <vector>   // vector
#include <utility>  // pair

int main() {
  std::vector<std::pair<std::string,std::string>> qa;
  
  {
    std::ifstream qifs("qus.txt");
    std::ifstream aifs("ans.txt");
    if ( !qifs.is_open() || !aifs.is_open() ) {
        return -1;
    }
    std::string qline, aline;

    while ( getline(qifs, qline) && getline(aifs, aline) ) {
       qa.push_back(std::make_pair(qline,aline));
    }
  }

  int i = 1;
  for ( auto& qa_pair : qa ) {
    std::string you;
    std::cout << i << "." << qa_pair.first << " ? " << std::flush;
    std::cin >> you;
    if ( you == qa_pair.second ) {
      std::cout << "〇\n";
    } else {
      std::cout << "× " << qa_pair.second << std::endl;
    }
    ++i;
  }
}

Comments

[Mitacchi]

質問と答えを一緒にするのは僕も考えましたが、少しややこしくなってあきらめていました...
理解できればこちらのほうがスマートそうですね
勉強の参考にさせていただきます！回答ありがとうございました！

Answer 2

指摘するなら次の通りでしょうか。

• 変数定義をグローバルにしているのは何故ですか？必要性がないならばローカル変数として定義すべきです。
• ループの回し方が変です。これでは最後の問題が出力されません。

 for (int i = 1; i < 200; i++) {
   cout << i << "." << qus[i - 1] << "\n";

こうすべきでは？

 for (int i = 0; i < 200; i++) {
   cout << i << "." << qus[i] << "\n";

また、問題と答えの行数が200行に満たない場合も200個のループが回りますが問題ないですか？

• qusとansにいれるデータの行数が等しいかチェックする処理を入れるべきです。同様に200行以上のデータを受け付けないようにする処理も入れるべきです。

Comments

[Mitacchi]

漠然とした質問に回答いただきありがとうございます！
変数の定義についてもう少し考えてみます！