オブジェクトの単一責任とは
ここからは実際のリファクタリングの実例です。
今回は以下の機能について。
選択した画像データを開くと、
画像データをグラフィックチップリストとして読み込むというものです。
ジグソーパズルのピースみたいな感じ。
上記は旧版。
新版も同様に実装。
ソースコード見ていきましょう。
旧 ↓
' GraphicChip1.vb
Public Class GraphicChip1
''' GraphicChip1 クラスのインスタンスを定義すると実行されます。コンストラクタ。 Public Sub New()
' GraphicChip1オブジェクト作成時の初期化処理を追加します。
End Sub
''' チップリストを生成します。 ''' チップリストを作成する元イメージとなるファイルパス。
''' 作成するチップリストの列数を指定します。
''' 作成するチップリストの行数を指定します。
''' チップリストを表示するボタンの形式を指定します。
''' チップリスト生成が正常に終了したかどうかを判別する Boolean 値。
Public Function Load_ChipList(ByVal imgFile As String, ByVal col As Integer, ByVal row As Integer, Optional ByVal fs As Boolean = False) As Boolean
Const ABS_LIST_SIZE As Integer = 48.0!
Const CHIP_LIST_MAX As Integer = 400
Const BUTTON_SIZE As Integer = 40
Dim panel As TableLayoutPanel = Form1.TableLayoutPanel1
Dim img As Image
' Process will check whether it is possible.
Try
img = Image.FromFile(imgFile)
Catch ex As IO.FileNotFoundException
MessageBox.Show("ファイルが見つかりません。処理を中断しました。", "通知")
Return False
Catch eex As Exception
MessageBox.Show("不明なエラーが発生したため処理を中断しました。", "通知")
Return False
End Try
If col * row > 400 Then
MessageBox.Show("チップ数が上限をオーバーしたため処理を継続出来なくなりました。チップ数の上限は「400」です。", "通知")
Return False
End If
' Create TableLayoutPanel1.
Dim columnvol = col
Dim rowvol = row
panel.ColumnCount = columnvol
panel.ColumnStyles.Clear()
For g As Integer = 0 To columnvol - 1
panel.ColumnStyles.Add(New ColumnStyle(SizeType.Absolute, ABS_LIST_SIZE))
Next
panel.RowCount = rowvol
panel.RowStyles.Clear()
For h As Integer = 0 To rowvol - 1
panel.RowStyles.Add(New RowStyle(SizeType.Absolute, ABS_LIST_SIZE))
Next
panel.Size = New Size(columnvol * ABS_LIST_SIZE, rowvol * ABS_LIST_SIZE)
Dim canvas(CHIP_LIST_MAX) As Bitmap
Dim graphicdata(CHIP_LIST_MAX) As Graphics
' この部分いらないはずです(要確認)
For h As Integer = 0 To CHIP_LIST_MAX
canvas(h) = New Bitmap(48, 48)
graphicdata(h) = Graphics.FromImage(canvas(h))
Next
' Divides the read image file and displays it sequentially in the individual space of TableLayoutPanel1.
Dim cpDivX As Integer = 0, cpDivY As Integer = 0, k As Integer = 0, propertyFlatStyle As FlatStyle, propertyFlatBorder As Integer
If fs = True Then
propertyFlatStyle = FlatStyle.Standard
propertyFlatBorder = 2
Else ' Default.
propertyFlatStyle = FlatStyle.Flat
propertyFlatBorder = 0
End If
Dim rest, src As Rectangle
For i As Integer = 0 To rowvol - 1
For j As Integer = 0 To columnvol - 1
Dim newButton = New Button
With newButton
.Name = "Chip" & k
.FlatStyle = FlatStyle.Flat
.FlatAppearance.BorderSize = 0
.Width = BUTTON_SIZE
.Height = BUTTON_SIZE
.BackColor = Color.Transparent
.BackgroundImageLayout = ImageLayout.None
.Tag = k
End With
cpDivX = ((j Mod columnvol) \ 1) * 16
cpDivY = i * 16
rest = New Rectangle(0, 0, 32, 32)
src = New Rectangle(cpDivX, cpDivY, 16, 16) 'Specify the size of the image chip.
graphicdata(k).DrawImage(img, rest, src, GraphicsUnit.Pixel)
newButton.BackgroundImage = canvas(k)
FAMESharedClasses1.Add_ChipData(newButton.BackgroundImage)
panel.Controls.Add(newButton, j, i)
panel.Invalidate()
AddHandler newButton.Click, AddressOf Func_newButton_Click
k += 1
Next
Next
Form1.TableLayoutPanel1 = panel
Form1.ChipEnterButton.Text = "リスト消去"
Return True
End Function
''' 表示しているチップリストを消去してリセットします。 Public Sub Clear_ChipList()
If Form1.ChipEnterButton.Text = "リスト消去" Then
'TableLayoutPanel reset.
Form1.TableLayoutPanel1.Controls.Clear()
FAMESharedClasses1.Clear_ChipData()
Form1.ChipEnterButton.Text = "グラフィック取込..."
End If
End Sub
''' Write processing from here when clicking on the created tip button. Private Sub Func_newButton_Click(sender As Object, e As EventArgs)
Dim chipnumber = CType(sender.Tag, Integer)
Form1.PictureBox1.BackgroundImage = FAMESharedClasses1.ChipData(chipnumber)
Form1.PictureBox1.Tag = chipnumber
End Sub
End Class
原文ままです。
新 ↓
// GraphicListFile.cs
/* using namespace */
using MapEditor.src.common;
using MapEditor.src.common.constants;
/* sources */
namespace MapEditor.src.app.IO
{
/// Image list file manager internal class GraphicListFile : IUserFileIO
{
/// Graphic chip data file path public string FilePath { protected set; get; }
/// Graphic chip image private Image? _image = null;
/// graphic chip list object data
public List ImageList { get; set; } = new();
internal GraphicListFile()
{
FilePath = string.Empty;
ImageList.Clear();
}
/// Loads the specified graphics data into read memory. /// Specify the full path of the graphicdata file public void FileOpen(string path)
{
_image = Image.FromFile(path);
if (_image != null)
{
FilePath = path;
}
}
/// Clears the data loaded in memory. /// Target file path public void FileClose(string path)
{
if (_image != null && path == FilePath)
{
_image?.Dispose();
ImageList.Clear();
}
}
/// Gets an empty of the specified size. /// Specifies the number of rows in the to construct /// Specifies the number of columns in the to construct
/// cell size (square absolute value)
/// instance /// Create and get a button control object for the graphics chip list. /// Prefix number to append to button names
/// Bitmap information to set in the background of the button
/// instance /// Get the object for the graphics chip list. /// Setting rows on the table /// Setting columns on the table
/// instance 原文ままです。
メソッドが違いますね。
超簡単に図式化すると以下です。
雑で申し訳ないです💦
私、人に教えるのとか解説みたいな説明技術ないので勘弁して😥
話の主軸はLoad_ChipListメソッドを4つのメソッドに細分化できたという話です。
上記の新旧は同じBehaviorを実行します。
まずは旧ソースの全体を俯瞰した時にforループとか同じような変数が数回登場していたりと読みにくいですね。
読みにくい原因としては、処理の区切りが分からないからどこまでが処理の一連なのかが判断できないのが問題。
これを根本的に解決するにはやはり処理を一つのメソッドに切り出すしかないはずなのだが、このソースコードを作った人はそんな事もお構いなしで処理を全部Load_ChipListメソッドに書き切っている。
LoadGraphicChipOnDrawCanvasメソッドの中に旧コードに含まれていた、
' Create TableLayoutPanel1.
Dim columnvol = col
Dim rowvol = row
panel.ColumnCount = columnvol
panel.ColumnStyles.Clear()
For g As Integer = 0 To columnvol - 1
panel.ColumnStyles.Add(New ColumnStyle(SizeType.Absolute, ABS_LIST_SIZE))
Next
panel.RowCount = rowvol
panel.RowStyles.Clear()
For h As Integer = 0 To rowvol - 1
panel.RowStyles.Add(New RowStyle(SizeType.Absolute, ABS_LIST_SIZE))
Next
panel.Size = New Size(columnvol * ABS_LIST_SIZE, rowvol * ABS_LIST_SIZE)の部分を切り出して、
private static TableLayoutPanel GetLayoutPanel(uint rowcount, uint columncount, int larger)
{
TableLayoutPanel table = new()
{
AutoSize = true,
RowCount = (int)rowcount,
ColumnCount = (int)columncount,
};
table.RowStyles.Clear();
for (var row = 0; row < table.RowCount; row++)
{
table.RowStyles.Add(new RowStyle(SizeType.Absolute, larger));
}
table.ColumnStyles.Clear();
for (var col = 0; col < table.ColumnCount; col++)
{
table.ColumnStyles.Add(new ColumnStyle(SizeType.Absolute, larger));
}
table.Size = new Size(table.ColumnCount * larger, table.RowCount * larger);
return table;
}// Create a resource to place the graphics chip list.
table = GetLayoutPanel(rows, cols, cell_size);に変更。
これでTableLayoutPanelオブジェクトを生成している部分のBegin and Endポイントが一目で分かるようになり、修正する時にもそのメソッドだけを直せば影響は少ないでしょう。
一つのメソッドには、一つの機能。
それって基本だと思うんです。
だって、テレビの電源付けたら放送が見れて、電卓も使えて、洗濯もできて、空調管理もできてって、そんな訳ないですよね。
それと同じこと。
同じ要領で、FileOpenメソッド、GetButtonメソッドも切り出して、チップリスト生成メソッド本体の行数を86行→32行に簡略化。
あとよく分からない、
' この部分いらないはずです(要確認)
For h As Integer = 0 To CHIP_LIST_MAX
canvas(h) = New Bitmap(48, 48)
graphicdata(h) = Graphics.FromImage(canvas(h))
NextIf fs = True Then
propertyFlatStyle = FlatStyle.Standard
propertyFlatBorder = 2
Else ' Default.
propertyFlatStyle = FlatStyle.Flat
propertyFlatBorder = 0
End Ifを添削しました。
更に言うとマジックナンバーという初学者が最初のうちだけしているような事を平気にしている部分があり、それらは見ていて著しく自己嫌悪に陥りました。。。
どうして・・・🤔
旧ソースのLoad_ChipListはBoolean型のメソッドになっており、実際には生成したTableLayoutPanelをMainFormのインスタンスに直接代入しているようです。
それで一点気になった部分があって、
Dim panel As TableLayoutPanel = Form1.TableLayoutPanel1これなんですが、C#で同じ事をしようとすると「TableLayoutPanelはアクセスできない保護レベルになっています」となり、代入できませんでした。
旧ソースの代入部分を調べてみると、Friend WithEvents ～ が Form1 フォームデザイナーに書かれていました。
これをC#で同じように書く事はできないようです。
つまりそのままVB.NETのソースコードをC#に移植する事ができない事を意味するのですがこのFriend WithEvents、よく知らないんですがどのクラスからでもアクセス出来てしまうように見えたので私はあまり好きではないです・・・
Friend WithEvents自体が何なのか、どうやってこんな処理を当時の私が見つけたのか分かりませんが・・・
ChatGPTに聞いても無理だと言われたので恐らく無理なのではないかと思われます。
コメント