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

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

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

Go(golang)は、Googleで開発されたオープンソースのプログラミング言語です。

コードレビュー

コードレビューは、ソフトウェア開発の一工程で、 ソースコードの検査を行い、開発工程で見過ごされた誤りを検出する事で、 ソフトウェア品質を高めるためのものです。

Q&A

解決済

1回答

1732閲覧

[Go]このコードを綺麗にするには?

miraiLABO

総合スコア10

Go

Go(golang)は、Googleで開発されたオープンソースのプログラミング言語です。

コードレビュー

コードレビューは、ソフトウェア開発の一工程で、 ソースコードの検査を行い、開発工程で見過ごされた誤りを検出する事で、 ソフトウェア品質を高めるためのものです。

0グッド

0クリップ

投稿2021/09/12 06:01

編集2021/09/12 10:34

綺麗なコードが分かりません。

このコードの汚い箇所・構造、等アドバイスお願いします。
Goで書いたコード管理ツールです。

※追記しました。

追記

抽象的な質問失礼しました。
今までチーム開発の経験が無く、私は汚いと感じないのですが、他人から見た見やすいコードとは何だろうと思い質問しました。

気になった事

構造体

構造体を使った方が分かりやすいでしょうか?

go

1type cmd_line struct { 2 Cmd string 3 Opition string 4 Name string 5}

今は入力を全て配列に突っ込んでしまっています。
私は、分散すると行ったり来たりで分かりにくいと感じます。しかし、他人のコードを見ると構造体がよく使われています。
また、他に構造体にした方が良い箇所はありますか?

同じような動作をどちらかに統一すべきか

exec.Command("cat"~)os.Open()を、os.Open()にまとめるべきでしょうか

関数の分け方

コマンドごとでしか関数を分けていませんが、cat,lsのコマンド呼び出し部分を一つにまとめた方が良いでしょうか?
2行程度なのでまとめ無くて良いかな〜と思っちゃいます。

質問として不適切かもしれませんが、Goをやっている、かつ、不特定多数に聞ける。のはここしか無いと思い質問させて頂きました。
Goをやっている人が周りに居ないもので...

コード

main.go

go

1package main 2 3import ( 4 "atCoder/BeginnersSelection" 5 "bufio" 6 "fmt" 7 "os" 8 "os/exec" 9 "strings" 10) 11 12// パッケージディレクトリのパス 13var dir_path string = "BeginnersSelection/" 14 15// ループフラグ 16var fl bool = true 17 18func main() { 19 fmt.Println(" hakadoru へようこそ") 20 fmt.Println(" hakadoru はAtCoderコード管理ツールです。") 21 fmt.Println(" 試しに `man` コマンドでマニュアルを見てみましょう! ") 22 fmt.Printf("\n\n") 23 24 // 動作部分。flがfalseで終了(100回の制限あり) 25 for i := 0; fl && i < 100; i++ { 26 fmt.Printf("hakadoru-command > ") 27 28 var sc = bufio.NewScanner(os.Stdin) 29 sc.Scan() 30 st := sc.Text() 31 arr := strings.Split(st, " ") 32 33 command_run(arr) 34 } 35} 36 37// 各コマンドの選択 38func command_run(arr []string) { 39 cmd := arr[0] 40 switch cmd { 41 case "man": 42 manual() 43 case "run": 44 run(arr) 45 case "cat": 46 cat(arr) 47 case "list": 48 list_view(dir_path) 49 case "exit": 50 fl = false 51 default: 52 fmt.Printf("command not found: %s \n", arr) 53 } 54} 55 56// `cat`コマンド動作部分 57func cat(arr []string) { 58 var result string 59 // sedオプション時 60 if arr[1] == "-sed" { 61 cat := exec.Command("cat", dir_path+arr[2]+".go") 62 var cat_result, err = cat.Output() 63 tmp := strings.Replace(dir_path, "/", "", -1) 64 result = string(cat_result) 65 result = strings.Replace(result, tmp, "main", -1) 66 tmp = strings.Replace(arr[2], arr[2][:1], strings.ToUpper(arr[2][:1]), -1) 67 result = strings.Replace(result, tmp, "main", -1) 68 69 if err != nil { 70 fmt.Println("err: ", err) 71 } 72 73 } else { 74 // catオプション無い時 75 cat := exec.Command("cat", dir_path+arr[1]+".go") 76 var cat_result, err = cat.Output() 77 result = string(cat_result) 78 79 if err != nil { 80 fmt.Println("err: ", err) 81 } 82 } 83 fmt.Println(result) 84} 85 86// `list`コマンド動作部分 87func list_view(path string) { 88 cmd := exec.Command("ls", path) 89 var ls_result, err = cmd.Output() 90 var str string = string(ls_result) 91 92 if err != nil { 93 fmt.Println("err: ", err) 94 } 95 96 fmt.Println("<ファイル一覧>") 97 slice := strings.Split(str, "\n") 98 for _, str := range slice { 99 if str != "" { 100 str = strings.Replace(str, ".go", "", -1) 101 fmt.Println(str) 102 } 103 } 104} 105 106// `run`コマンド動作部分 107// が増える度に追記する。 108func run(arr []string) { 109 sel := arr[1] 110 fmt.Printf("running : %s\n", sel) 111 112 switch sel { 113 case "q": //quit 114 case "practiceA": 115 BeginnersSelection.PracticeA() 116 case "abc086a": 117 BeginnersSelection.Abc086a() 118 case "abc081a": 119 BeginnersSelection.Abc081a() 120 case "abc081b": 121 BeginnersSelection.Abc081b() 122 case "abc087b": 123 BeginnersSelection.Abc087b() 124 case "abc083b": 125 BeginnersSelection.Abc083b() 126 case "abc088b": 127 BeginnersSelection.Abc088b() 128 default: 129 fmt.Printf(">%s does not exist \n", sel) 130 } 131} 132 133func manual() { 134 var result string 135 136 str, err := os.Open("manual.txt") 137 if err != nil { 138 fmt.Println(err) 139 } 140 defer str.Close() 141 142 tmp_slice := make([]byte, 1024) 143 for { 144 n, err := str.Read(tmp_slice) 145 if n == 0 { 146 break 147 } 148 if err != nil { 149 break 150 } 151 result += string(tmp_slice[:n]) + "\n" 152 } 153 fmt.Println(result) 154}

ディレクトリ構造

diff

1atCoder/ 2├── BeginnersSelection 3│ ├── abc081a.go 4│ ├── abc081b.go 5│ ├── abc083b.go 6│ ├── abc086a.go 7│ ├── abc087b.go 8│ ├── abc088b.go 9│ └── practiceA.go 10├── README.md 11├── go.mod 12├── main.go 13└── manual.txt

説明

AtCoderの問題別コードをを管理するツール

リポジトリ:https://github.com/null-miraiLABO/AtCoder-Go

流れ

1.atCoder ディレクトリ上で go run main.go をする。

2.各コマンドの実行

3.exitで終了

cmdマニュアル

diff

1//実装済みの全コマンド 2 3 man //マニュアルの表示 4 5 list //パッケージ内ファイル一覧 6 7 run [パッケージ名] //選択したパッケージを実行 8 9 cat [オプション] [パッケージ名] //選択したパッケージのコードを表示 10 -sed //選択したパッケージをコードをAtCoder提出用に文字列置換し表示 11 12 exit //終了。 13 14//記入例 15 cat -sed abc083b 16

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

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

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

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

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

y_waiwai

2021/09/12 06:25 編集

あなたは、このコードのどの部分が「汚い」と感じるんでしょうか あるいは、どの部分をどうにかしたいと思ってるんでしょう
gentaro

2021/09/12 07:04

ここはあなたを教育するための無料相談所ではないので、「困っていること」を明確にして質問しよう。 このコードの問題点を示す義務はあなたの側にあるので、それができないのに「キレイにしてくれ」というのは質問にすらなってない。
miraiLABO

2021/09/12 10:35

質問内容に追記しました。ご回答頂けると助かります。
guest

回答1

0

ベストアンサー

(私はあまりGoは詳しくないですが)

Goをやっている人が周りに居ないもので...

こういうのは大抵、言語には依存しない事が多いです。
仮に依存していても、調べればいいだけです。

たとえば、「ク○コード」を反面教師にするとか。

構造体を使った方が分かりやすいでしょうか

場合によります。

質問にあるコード(の断片)だと、

Go

1type cmd_line struct { 2 Cmd string 3 Opition string 4 Name string 5}

だと、Cmd や Option, Name はそれぞれ関係のあるものでしょうか。

相当アレなところだと、「関係のないものも、別の関数で使うから構造体にする」とかっていう人もいるようです。それはナンセンスですね。

構造体の役割は、複数のデータをひとまとめにして、意味を持たせるためです。

たとえば、

CSVファイル( カンマを区切り文字にした、Excelっぽいファイル ) を読み込んでソートするとかの場合とかですね。

学籍番号 - 名前 - 数学の点数 - 国語の点数 - 英語の点数

で一行分(= 1人分 ) として、

101,田中太郎,30,40,50 102,田中次郎,100,100,100 103,山田三郎,100,0,100 104, ...

と言う風にあったとします。

これをソートして表示するとかそういう場合、

name1 string, name2 string... のように別々の変数にしたり、
names[] string, numbers[] int ... のように別々の配列にしたりするよりも、

Person構造体を用意して、

Go

1type Person struct { 2 number int // 学籍番号 3 name string 4 math int 5 japanese int 6 english int 7}

とかみたいにして、「1人分のデータ」として、それの配列とかを持つ。

でも、この構造体に、(他の関数とかで使うからって)関係のない「コマンドライン引数」を持つとかだとダメ。
まとめて意味を持たせる

後は『コーディング規約』とかで検索してください。
かなり議論されていますから。

投稿2021/09/12 12:09

編集2021/09/12 12:17
BeatStar

総合スコア4958

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

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

あなたの回答

tips

太字

斜体

打ち消し線

見出し

引用テキストの挿入

コードの挿入

リンクの挿入

リストの挿入

番号リストの挿入

表の挿入

水平線の挿入

プレビュー

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

ただいまの回答率
85.48%

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

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

質問する

関連した質問