質問をすることでしか得られない、回答やアドバイスがある。

15分調べてもわからないことは、質問しよう!

新規登録して質問してみよう
ただいま回答率
85.53%
Unity

Unityは、Unity Technologiesが開発・販売している、IDEを内蔵するゲームエンジンです。主にC#を用いたプログラミングでコンテンツの開発が可能です。

Q&A

解決済

2回答

1093閲覧

実装できたけどコードが汚い時

RyojiAraki

総合スコア79

Unity

Unityは、Unity Technologiesが開発・販売している、IDEを内蔵するゲームエンジンです。主にC#を用いたプログラミングでコンテンツの開発が可能です。

0グッド

0クリップ

投稿2023/01/25 19:32

編集2023/01/27 23:50

エラーではないのですが

「やりたかったことを実装できたけど、コードが汚い...もしかしたらもっと綺麗に書けるかもしれない...」って時、皆さんはどうされてますか?
私の場合、実装はできるんですが、結局綺麗にできず苦しい思いをしています。
伸び代を逃しているようで、結構深刻に悩んでいます。

実現したいこと

皆さんの意見を聞かせてください

今のコード

行数の多さが今気になってます。

c#

1using System.Collections; 2using System.Collections.Generic; 3using static System.Collections.IEnumerable; 4using System; 5using System.Linq; 6using UnityEngine; 7using UnityEngine.UI; 8using TMPro; 9 10public class InventoryManagement : MonoBehaviour 11{ 12 public ItemInInventory[] skill_inventory = new ItemInInventory[6]; 13 public ItemInInventory[] item_inventory = new ItemInInventory[16]; 14 15 public ItemDataBase database; 16 public Category item_category; 17 18 public GameObject[] skill_iconBox = new GameObject[6]; 19 public GameObject[] item_iconBox = new GameObject[16]; 20 public GameObject text; 21 private TextMeshProUGUI _text; 22 23 public struct ItemInInventory 24 { 25 public string id; 26 public string type; 27 public int cooldown; 28 public int ability_haste; 29 public int arm_power; 30 public int absolute_dig; 31 public float ratio_dig; 32 public int money_invested; 33 public bool availability; 34 35 public ItemInInventory(string id,string type, int cooldown, int ability_haste, int arm_power, int absolute_dig, float ratio_dig, int money_invested, bool availability) 36 { 37 this.id = id; 38 this.type = type; 39 this.cooldown = cooldown; 40 this.ability_haste = ability_haste; 41 this.arm_power = arm_power; 42 this.absolute_dig = absolute_dig; 43 this.ratio_dig = ratio_dig; 44 this.money_invested = money_invested; 45 this.availability = availability; 46 } 47 } 48 49 void Start() 50 { 51 _text = text.GetComponent<TextMeshProUGUI>(); 52 DisplayItems(); 53 DebugCheck(); 54 } 55 56 private void DebugCheck() 57 { 58 AddNewItems("pickaxe_diamond_1"); 59 AddNewItems("pickaxe_wood_1"); 60 AddNewItems("runner"); 61 } 62 63 private ItemInInventory ConvertIDintoStructedItem(string newID) 64 { 65 string id = newID; 66 Item item = SearchItemFromID(id); 67 return new ItemInInventory(id,item.type.ToString(),item.cooldown,item.ability_haste,item.arm_power,item.absolute_dig,item.ratio_dig,item.money_invested,true); 68 } 69 70 public void DisplayItems() 71 { 72 AssignImageFromArray(item_inventory); 73 AssignImageFromArray(skill_inventory); 74 } 75 76 private void AssignImageFromArray(ItemInInventory[] array) 77 { 78 var i = 0; 79 foreach(ItemInInventory item in array) 80 { 81 if(item_iconBox[i] != null)//まだiconBoxの割り当てが済んでいないため あとでやる(やらないパターン) 82 { 83 if(item.id != "") 84 { 85 if(array.Length == 6){skill_iconBox[i].GetComponent<Image>().sprite = SearchItemFromID(item.id).item_image;}//parameterの配列がskillかitem由来か確認 86 else{item_iconBox[i].GetComponent<Image>().sprite = SearchItemFromID(item.id).item_image;} 87 } 88 else 89 { 90 if(array.Length == 6){skill_iconBox[i].GetComponent<Image>().sprite = null;} 91 else{item_iconBox[i].GetComponent<Image>().sprite = null;} 92 93 } 94 } 95 i++; 96 } 97 } 98 99 public void DisplayItemDescription(int i) 100 { 101 Item selected_item; 102 if(i >= 6) 103 { 104 selected_item = SearchItemFromID(item_inventory[i-6].id); 105 } 106 else 107 { 108 selected_item = SearchItemFromID(skill_inventory[i].id); 109 } 110 if(selected_item!=null) 111 { 112 _text.text = GenerateText(selected_item,"others"); 113 } 114 115 } 116 117 private string GenerateText(Item item, string _availability) 118 { 119 string name; string text_parameter; string num_parameter; string type; string item_info; 120 121 name = "<size=20>" + item.item_name + "</size>"; 122 123 text_parameter = "<size=16>" + SearchCategoryFromItem(item,"text") + "</size>"; 124 125 num_parameter = ""; 126 127 type = ""; 128 129 if(_availability == "others") 130 { 131 type = "<size=16>" + item.type.ToString() + "</size>"; 132 } 133 else if(_availability == "true") 134 { 135 type = "<size=16>" + "Available" + "</size>"; 136 } 137 else if(_availability == "false") 138 { 139 type = "<size=16>" + "Unavailable" + "</size>"; 140 } 141 142 item_info = "<size=10> \n" + item.information + "</size>"; 143 144 145 return name + text_parameter + num_parameter + type + item_info; 146 } 147 148 149 private string SearchCategoryFromItem(Item item,string purpose) 150 { 151 if(purpose == "text") 152 { 153 if(item_category.Diggable.Contains(item)) 154 { 155 return "Diggable:"; 156 } 157 else if(item_category.Investable.Contains(item)) 158 { 159 return "Investable:"; 160 } 161 } 162 else if(purpose == "unit") 163 { 164 if(item_category.Unit_KiloTon.Contains(item)) 165 { 166 return "kt"; 167 } 168 } 169 170 return ""; 171 } 172 173 public void ResetText() 174 { 175 176 } 177 178 public void AddNewItems(string newID) 179 { 180 //List<ItemInInventory> list; 181 ItemInInventory[] array; 182 183 if(SearchItemFromID(newID).type.ToString() == "Skill"){array = skill_inventory;} 184 else{array = item_inventory;} 185 186 int index = Array.IndexOf(array, null); 187 if (index >= 0) array[index] = ConvertIDintoStructedItem(newID); 188 189 DisplayItems(); 190 } 191 192 public void ItemUsed(int i, GameObject icon) 193 { 194 Item selected_item = SearchItemFromID(item_inventory[i].id); 195 196 if(selected_item.type.ToString()=="Reusable") 197 { 198 icon.GetComponent<Image>().color = new Color32(58,58,58,255); 199 } 200 } 201 202 public void SkillUsed(int i, GameObject icon) 203 { 204 Debug.Log(SearchItemFromID(skill_inventory[i].id).item_name); 205 //ここに効果発動させるコマンドと、周りを光の尾で照らすコマンドをかく 206 } 207 208 private Item SearchItemFromID(string given_ID) 209 { 210 Item selected_item; 211 212 foreach(Item item in database.items) 213 { 214 if(item.item_id == given_ID) 215 { 216 selected_item = item; 217 return selected_item; 218 } 219 } 220 221 return null; 222 223 } 224 225} 226 227

気になる質問をクリップする

クリップした質問は、後からいつでもMYページで確認できます。

またクリップした質問に回答があった際、通知やメールを受け取ることができます。

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

dameo

2023/01/25 21:37

主観や嗜好が強く出る上に、そもそもが程度問題なので、実際のコードがないと私には何も言えません。
RyojiAraki

2023/01/25 21:44

前のやつ全部綺麗に修正しちゃったので、またコード汚くなったら投稿します! 私の症状、強迫性障がいの類などではないかと自分では疑ってます。
Zuishin

2023/01/25 22:37 編集

きれいに修正できるならいいのでは? 実装してリファクタリングするのはテスト駆動開発では当たり前にすることです。
ozwk

2023/01/25 23:32

> 実装はできるんですが、結局綺麗にできず苦しい思い > 前のやつ全部綺麗に修正しちゃった 綺麗にできるのかできないのかどっちなんですか?
pig_vba

2023/01/26 00:13

実装時点でコードが最適化できないという意味なら普通のことなので気にすることはないと思います。後からリファクタリングできてるなら十分でしょう。 初めからどこまできれいに書けるかは経験の差だと思うのでそこに時間使うより早く実装することを個人的に重視してるので参考にならないかもしれませんが
Zuishin

2023/01/26 00:21

どのくらい汚いのか見てみました。 https://teratail.com/questions/tryf45dtxywuem first second が多いのがまず目につく汚いところなので、配列やリストを使うことを覚えるのが良さそうです。
hogefugapiyo

2023/01/26 03:20

通常の実装に伴うリファクタリングはやったほうがいいですが基本的に動けばOKです。 そこをキレイに実装できるかを検討する部分に工数をかけていいのかっていう点をまず考えてもらえればと思います。
Zuishin

2023/01/26 03:26

工数をかけないようにしたいのであればもう少し基礎的な知識が必要です。 配列が使えないのではあまりに効率が悪すぎます。 どこに工数をかけるかを考える以前にまずプログラマーの性能を上げましょう。 初心者であればあるほどその効果は発揮されます。
RyojiAraki

2023/01/26 22:23 編集

>>ozwkさん 気合いで綺麗にしますが、かなり時間かかりますし完璧かわからないです、、 >>pig_vbaさん 私の父と言ってること一緒です。(細かいことよりまず完成させろって言ってました)これが普通なんですね、少し安心しました。たまに他の開発者さんのツイートとか見るんですが、自分が理解不能なコードの書き方とかしてて、しかも短く簡潔で、劣等感を覚えます。 ;-; 経験積むこと心掛けるようにします。 >>zuishinさん 昔のコードわざわざ見てくださりありがとうございます!心身になってくれて超うれしいです。今はだいぶ配列とか改善されてると思います!まだ共同開発の経験がないのですが、出来るだけリファクタリングしていつかの現場に備えるようにします。 >プログラマーの性能を上げましょう。 確かにそうですよね、、何かわからないことがあれば調べて、その場しのぎをするの連続が現状なので近いうちに改善したいところです。何かおすすめの基礎知識習得サイトがあれば紹介していただけると嬉しいです!できれば無料ので(すみません) >>hogefugapiyoさん そうなんです。。実はあんまり時間がなくてそこに時間を取るかどうか迷ってるってのが現状です。もし精神と時の部屋に行けたら、絶対リファクタリングしますが、、 >>みなさんへ 今のコード貼り付けてみます!行数を減らすコツとか教えてください。まだコード書いてる途中なのでnullチェックのif文とか、何も書いてない関数とかあります(完成次第消したり書き足したりする予定です) あと、皆さんが始めた頃、どんな機会があって簡潔に書けるようになったかとかのエピソードも共有していただけると嬉しいです。
Zuishin

2023/01/26 22:35 編集

> 行数を減らすコツとか教えてください。 同じことを何度も繰り返しているので、関数もしくはクラスを分離するのが良いと思います。 引数や総称体や高階関数を使うと分離しやすくなります。 たとえば ItemAdded と SkillAdded に使われているロジックは全く同じものなのでまとめることができます。 またこれは LINQ を使えば一行で書けます。 もっと言えば、配列ではなく List<string> を使えば自分でロジックを構築することすら不要です。
RyojiAraki

2023/01/26 23:01 編集

ありがとうございます!なぜそれに気付けたかったのか、自分が恥ずかしいです。 とりあえず、itemAddedとかDisplayitem修正してみました。 LINQの存在初めて知りました :0 使えたらめっちゃ便利そうなので、今から積極使ってみようと思います!Listは今の自分の書いてるのに当てはめるのが難しそう(収容数が規定されてて、空いてる箇所があれば追加する)なので時間まとまった時にチャレンジしてみます! ありがとうございます!!! 追記:プロフィール確認いたしました。13歳とは思えないほど大人びてて素敵です。 コードの全体修正が終わったら、自己解決のところに結果を投稿しようと思います。今回はありがとうございました。
Zuishin

2023/01/27 01:06 編集

ItemAdded と SkillAdded をまとめるというのは、一つの関数にできるという意味です。 public void AddNewID(string[] list, string newID) { var index = Array.IndexOf(list, ""); if (index >= 0) list[index] = newID; DisplayItems(); }
dameo

2023/01/26 23:41

ぱっと見ですが、単体のソースレベルでコードを追えないほど汚いわけではないので、個人的に気にすることはないかと思います。まずは動くことが大事です。 Zuishinさんのおっしゃるとおり、冗長な部分はあるので、コーディング終了時点で冗長な部分は残さないようにすべきです(ただし本当に共通化していいのかは確認できませんが)。 他のコードとの兼ね合いで決まるし、実装というよりは設計なので以下は確認していません。 ・カプセル化とその意味 ・マルチスレッドと同期 ・クラスの命名と役割分担 なお全ては目的があってすることなので、何にせよ目的に適っていれば、個人的にはさほど気にしなくていいと思います。こうでなくてはならないってことはないという話です。 以下は自分が実装する場合の個人的な感覚です。。。 必須なのは冗長な処理がなく、合理的に記述できていること。 必須条件を満たしつつより速いコードが書ける場合、綺麗さとの兼ね合いを考えつつ自分の考える最善のコードにしていくだけ。 コードをバージョン管理しながら、いろいろなパターンのコードを書いて試して最善(最速)を決める感じです。 最終的に自分でこれ以上綺麗にできないと思うところまで綺麗にしないとコーディングは終わりません。 あとは時間と要件次第です。 コーディングを始めた頃は、必要とあらばCPUコードを数字で直打ちしたりして、みんな💩コードを書いては実行してハングして喜ぶ牧歌的時代だったので、あまり参考にならないかも。綺麗さを気にし始めたのはコードで飯を食うようになってからで、オブジェクト志向を学んで本を読んでた頃ですね。 綺麗さというのには好みがあり、それを語るのは不毛だということを学んだのもこの頃です。 以降私は速度に傾倒し始めて今に至る感じでしょうか。。。
RyojiAraki

2023/01/27 23:45 編集

>Zunshinさん ここまでコード短縮できるのはヤバすぎる...本当にありがとうございます! var index = Array.IndexOf(list, ""); if (index >= 0) list[index] = newID; ここらへん、芸術作品の粋だと思います。冗談抜きで感動しました。 追記 Arrayが存在しないって言われちゃいました :'( 修正でき次第更新します! つい追記 using System;で解決しました >dameoさん 完全に共通化しちゃダメなのもあるので微調整しながらやってます!最速決めるの楽しそうですね〜 数字で直打ちは色々理解の度を超えている...01010101001みたいな2進数ってことですよね? :0 綺麗さより、合理性と速度を意識してやってみます!そっちの方が意識しやすいかも...? 体験談までありがとうございました!
Zuishin

2023/01/29 03:11

一つの関数にする時に、二つの関数を足してはいけません。 二つの関数の共通部分を抜き出しましょう。 私の書いたコードでは、共通していない部分を引数として与えています。 これを外部から与えず、関数内部で判定するのは非常に良くない方法で、これと比べるのであれば、元のコードの方が遥かに良いコードです。
RyojiAraki

2023/01/31 10:40

なるほどー なんとなく変更に弱そうだなって思いながら書いてました(アホ) 変えてみます!ありがとうございます!
guest

回答2

0

自己解決

他の方から頂いたコツをまとめました!

  • 実装時点で最適化できないのは当たり前
  • 基礎的な知識を補填する (LINQなど)
  • 余裕があればリファクタリングは必ずして経験を積む
  • 何度も繰り返す冗長な処理はクラス/引数を用いた関数で簡潔に制御
  • 合理性を意識する

みなさん本当にありがとうございました!
特にzunshinさんコードの修正まで手伝ってくれて感謝しかないです。
まだまだ初心者ですが頑張って最強プログラマー目指します!

投稿2023/02/04 19:11

RyojiAraki

総合スコア79

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

0

伸びしろを考えてるってことは
この後もずっとコードかくんでしょ
なら次にいかせばいいよ

汚いコードは汚いコードでいい
直す必要なし
もどるな
まえむけ
完成させろ

そう自分にいいきかせてやってる

投稿2023/02/04 02:51

ATTOMAN

総合スコア40

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

あなたの回答

tips

太字

斜体

打ち消し線

見出し

引用テキストの挿入

コードの挿入

リンクの挿入

リストの挿入

番号リストの挿入

表の挿入

水平線の挿入

プレビュー

15分調べてもわからないことは
teratailで質問しよう!

ただいまの回答率
85.53%

質問をまとめることで
思考を整理して素早く解決

テンプレート機能で
簡単に質問をまとめる

質問する

同じタグがついた質問を見る

Unity

Unityは、Unity Technologiesが開発・販売している、IDEを内蔵するゲームエンジンです。主にC#を用いたプログラミングでコンテンツの開発が可能です。