コードの書き方？マナー？に反していないかどうか

Question

前提・実現したいこと

いくつか引数を受け取り、その値から判定を行う。

発生している問題・エラーメッセージ

C++でプログラミングを行っているのですが、問題の個所がプログラミングの作法的に
おかしくないのか確認したいです。

該当のソースコード

CString クラス名::関数名(CString str1, CString str2, CString str3)
{
    if(str1.compare(設定値1))
    {
      return "状態１";
    }
    
    if(str2.compare(設定値2))
    {
      return "状態2";
    }
    
    if(str3.compare(設定値3))
    {
      return "状態3";
    }

　　return "";
}

試したこと

始めは関数の戻り値をBOOLで処理を行っていたのですが、
どの設定値で状態が遷移したかの情報が欲しく戻り値をCStringにしてみました。

補足情報（FW/ツールのバージョンなど）

本クラスはファイルから設定値を読み込み、
その値をメンバ変数として保持しておくクラスとなっております。
そこに設定値を使用した判定を行う関数を追加したのが上記のものとなっております。

Answer 1

こんにちは。

1. ワイド文字識別子名（クラス名、関数名など）

使っちゃいけないというわけでもないのですが、つい最近まで gcc では使えませんでしたし、海外のプログラマがプロジェクトに入ってくることもあると思います。
特にC++はインフラに近い方の開発が多いので、ワイド文字識別子を使わないプロジェクトが多いと思います。
ですので、識別子名はASCIIの範疇にしておいた方が好ましいと感じます。

1. ワイド文字列（"状態1"など）

ワイド文字はエンコードの問題（UTF-8やShift-JISなどなど）があるので、使わないで済むケースであれば使わない方が余計なトラブルが起きにくいです。

1. 条件判定に使用する値としての文字列（"状態1"など）

戻り値は、決まった複数の値の中から１つを返却できれば良いようなので、enum型の使用がお薦めです。文字列ですとタイプ・ミスが見つかるのは実行時です。テスト漏れすると実稼働時に初めてバグが見つかります。怖いですよね。
enum型なら多くのタイプ・ミスはコンパイル時に見つかります。安心感が段違いです。

1. 仮引数（CString型）

受け取って変更しないクラス型の仮引数はCString const&で受け取ると高速ですので、このようにconst参照渡しを使う人も多いと思います。
とはいえ、値渡しはコピーが発生するので遅いですが、それで特に問題がない状況もそれなりにあると思います。

Comments

[principle...]

文字列をそのまま返していることがすごいもやもやしていたのですが教えて
いただいたようにenum型で定義するのが良い感じなんですね…
参考にして修正させていただきます！
丁寧に教えていただきありがとうございました。

Answer 2

※ teratail C/C++警察が大挙してやってきそうな質問ですが、露払いとして。。。

CStringと言うことは、 Visual Studio のC++で、ATL/MFCをお使いですね？　パッと見、思いつく限りでは

//　もと
CString クラス名::関数名(CString str1, CString str2, CString str3)

CStringのクラスオブジェクトを値渡しと返り値に使っている。必然性の無い、無駄なコンストラクターが起動してしまう。あえてこのシグネチャの路線なら、私だったら返す値はCStringのポインターにして、他の引数は変更されないものなので、参照&を使います。また、メンバー関数としてはstaticで充分でしょうか。

// ぱっと考えた限りの改善案
static void クラス名::関数名(CString* returnedStr, const CString& str1, const CString& str2, const CString& str3)

複数の返り値が必要なら、別途enumを定義して返した方が良いのでは、と思いました。

Comments

[dodox86]

> teratail C/C++警察が大挙してやってきそうな
私は「自警団」くらいには入りたい思いですが。。。ちょっと無理かｗ

[principle...]

基本的なことがまだまだ勉強不足で回答いただいた内容まで考えられていませんでした…
参考にして改良させていただきます！
迅速な回答ありがとうございました。

[dodox86]

正しいと考えられるやり方をすると、分からない（≒受け入れない）人には「何これ？止めて！」と不評だったりしますから要注意です。constや参照を使うだけで拒絶されたり。そんな場合は啓蒙するか、自分の中のノウハウで留めておくしかないですね。

Answer 3

int クラス名::関数名(vector<CString> config)
{
    for (i = 0; i < config.size(); i++) {
        if (config[i].compare(設定値[i]) {
            return i;
        }
    }
    return -1;
}

じゃないですかね。

Comments

[Zuishin]

あるいは可変長引数の方が扱いやすいかもしれません。その場合戻り値から引数が復元できませんが。