関数の戻り値に複数の意味を持たせてはいけない

初心者向けアンチパターン紹介記事です（短め）

サンプルコードはC++で書きますが単純なものです。

戻り値に複数の意味があるとどうなる

まずいパターン

例えばこんな関数:

sample.cpp

bool getStr(int id, std::string& out)
{
	
	// id に紐づく文字列を取得する処理
	// 取得に失敗したら return false; する

	return true;
}

bool compareStr(const std::string &str, int id)
{
	std::string compareTarget;
	auto ret = getStr(id, compareTarget);
	if (ret == false) {
		return false;
	}

	return 0 == str.compare(compareTarget);
} 

この関数の問題点は、getStr()が何かしら失敗した場合も文字列比較の結果として一致しなかった場合もcompareStr()の戻り値が false になる点です。

冷静に考えてやばい関数設計ですが、compareStr()の呼び出し元が「文字列が一致したときだけ以降の処理をする」とかだった場合、問題なく仕様を満たしてしまいます。
後々になって、以下のように一致しなかったルートを追加してしまったら問題が起こることもあり得ます:

sample2.cpp

int func() 
{
	int id;
	std:string str;

	if (compareStr(str, id) == true) {
		// 文字列が一致したときの処理
	}
	else {
		// 文字列が一致しなかったときの処理
	}

	return 0;
}

else のルートでは文字列が一致しなかったときの処理を行っていますがcompareStr()から false が返されるのは文字列が一致しなかったときだけでなくgetStr()が失敗したときも、です。
まずいパターンとしてgetStr()の中の処理が何かの理由で失敗したけど、それが成功していれば本当は文字列は一致していた、というパターンが考えられます。期待していないタイミングで「文字列が一致しなかったときの処理」が走ることになります。

どうすべきか

少しでも複雑さを持った関数の戻り値に bool 値を使わないようにするべきです。
int を返すようにして、数字毎に一つの意味だけを割り当てるのが良いと思います。

sample3.cpp

int compareStr(const std::string &str, int id)
{
	std::string compareTarget;
	auto ret = getStr(id, compareTarget);
	if (ret == false) {
		return -1;
	}

	if (0 == str.compare(compareTarget)) {
                return 1;
        }
        else {
                return 0;
        }
} 

bool戻り値を使っていい場面は多くない

そもそも、bool を戻り値に使っていい場面はそうそうないと考えるべきです。
クラスが保持してるフラグを返すだけだったり、本当にシンプルな文字列一致を確認するだけだったり、
そういう場面でしか bool の戻り値は使わないようにするといいと思います。

sample4.cpp

bool someClass::getFlag() { return flag; }