改进这段丑陋的代码

Question

我在VB.net写一个二维码生成器

首先,我检查用户选择的(QR码版本)值.

每个版本具有固定的每个模式的位数(0001,0010,0100,1000).

基本上我现在得到的是以下内容:

Private Function get_number_of_bits() As Integer
    Dim bits As Integer

    If listVersion.Value < 10 Then
        If get_binary_mode(listMode.SelectedItem) = "0001" Then
            bits = 10
        End If
        If get_binary_mode(listMode.SelectedItem) = "0010" Then
            bits = 9
        End If
        If get_binary_mode(listMode.SelectedItem) = "0100" Or _
           get_binary_mode(listMode.SelectedItem) = "1000" Then
            bits = 8
        End If
    ElseIf listVersion.Value < 27 Then
        If get_binary_mode(listMode.SelectedItem) = "0001" Then
            bits = 12
        End If
        If get_binary_mode(listMode.SelectedItem) = "0010" Then
            bits = 11
        End If
        If get_binary_mode(listMode.SelectedItem) = "0100" Then
            bits = 16
        End If
        If get_binary_mode(listMode.SelectedItem) = "1000" Then
            bits = 10
        End If
    Else
        If get_binary_mode(listMode.SelectedItem) = "0001" Then
            bits = 14
        End If
        If get_binary_mode(listMode.SelectedItem) = "0010" Then
            bits = 13
        End If
        If get_binary_mode(listMode.SelectedItem) = "0100" Then
            bits = 16
        End If
        If get_binary_mode(listMode.SelectedItem) = "1000" Then
            bits = 12
        End If
    End If

    Return bits
End Function

哪个有效,但当然它是一个丑陋的.....代码:)

写这个更好的方法是什么？

编辑

按照要求.

listMode是一个组合框,填充有:

Private Function get_encoding_modes() As Dictionary(Of String, String)
    Dim modes As New Dictionary(Of String, String)
    modes.Add("0000", "<Auto select>")
    modes.Add("0001", "Numeric (max. 7089 chars)")
    modes.Add("0010", "Alphanumeric (max. 4296 chars)")
    modes.Add("0100", "Binary [8 bits] (max. 2953 chars)")
    modes.Add("1000", "Kanji/Kana (max. 1817 chars)")

    Return modes
End Function

get_binarymode的代码

Private Function get_binary_mode(ByVal mode As String) As String
    Dim modes As New Dictionary(Of String, String)
    modes = get_encoding_modes()

    Dim result As String = ""

    Dim pair As KeyValuePair(Of String, String)
    For Each pair In modes
        If pair.Value = mode Then
            result = pair.Key
        End If
    Next

    Return result
End Function

Answer 1

" TL; DR女孩"来救援!使代码不那么难看!了解了VB中的LINQ,以及如何做(而不是做)Lambdas.删除了反向字典搜索的需要,删除了很多重复自己,并使事情通常愉快的工作.:)？

好吧,我想对此进行一次尝试,即使Visual Basic不是我常用的东西.然而,有些事我认为我可以改进,所以我决定采取刺.首先,我创建了一个解决方案并实现了一些基本功能,因为我不熟悉VB,并认为这样做会更容易.如果您有兴趣,可以在这里找到整个解决方案:UglyCode-7128139.zip

我创建了一个小样本表单并尝试包含我可以从代码中收集的所有内容.这是一个截图:

这个小应用程序就是我用来测试代码的全部内容; 但即使我用它作为我编写的代码的目标,我想我想出了一些很好的方法来处理那些容易变得更通用的东西.所有这些目前都在主表单代码文件中实现,但没有什么能阻止它被引入一些更通用的帮助程序类.

首先,我处理了查找.我不确定的一件事是可以选择的第三个版本的数量,因为它涵盖了问题中Else第一个重要If声明的部分.这对我创建的查找不是问题,因为只有一个未知值.我选择0但如果有的话,它应该更新到真正的价值.如果它只是一个默认值,那么0对我们来说应该没问题.

我想首先,让我们看一下比特数的查找,以及编码的查找:

查找

You'll note that my lookups and a helper function are declared as Public Shared. I did this because: *neither the lookups nor the helper function requires knowing anything about a specific instance of the class it belongs to. *it allows you to create the item once for the entire application, so you avoid having to create it anew each time. *multiple creations isn't an issue for this application, but I just did it on principle: for large lookups and/or applications which created many instances of a class containing a lookup, the memory and processing requirements can become burdensome.

BitsLookup:

' Maps from a Version and Mode to a Number of Bits
Public Shared BitsLookup _
    As New Dictionary(Of Tuple(Of Integer, String), Integer) From
{
    {VersionAndMode(10, "0001"), 10},
    {VersionAndMode(10, "0010"), 9},
    {VersionAndMode(10, "0100"), 8},
    {VersionAndMode(10, "1000"), 8},
    {VersionAndMode(27, "0001"), 12},
    {VersionAndMode(27, "0010"), 11},
    {VersionAndMode(27, "0100"), 16},
    {VersionAndMode(27, "1000"), 10},
    {VersionAndMode(0, "0001"), 14},
    {VersionAndMode(0, "0010"), 13},
    {VersionAndMode(0, "0100"), 16},
    {VersionAndMode(0, "1000"), 12}
}

The idea here is very simple: instead of representing the lookup in a procedural manner, via the big If statement or via Select Case, I tried to see what the code was really doing. And from what I can tell this is it. Representing it in this kind of structure seems to me to fit better with the actual meaning of the data, and it allows us to express our ideas declaratively (what to do) rather that imperatively (how to do it). VB's syntax is a little bulky here but lets go through the parts.

Tuple(Of Integer, String)

A Tuple is just a convenient way to group data items. Until I saw it in .NET I had only heard of it in the context of a relational database. In a relational database a Tuple is approximately equivalent to a row in a table. There are a few differences, but I'll avoid going off-track here. Just be sure you know that a Tuple is not always used in the same sense as it is here.

But, in this case, it seemed to me that the data was organized as a lookup of the number of bits, based upon both the version and mode. Which comes first is not really relevant here, since either here (or in any procedural lookup), we could just as easily reverse the order of the items without it making a difference.

So, there is a unique "thing" that determines the number of bits, and that "thing" is the combination of both. And, a perfect collection type to use when you have a unique thing (Key) that lets you look up something else (Value) is of course the Dictionary. Also note that, a Dictionary represents something very much like a database table with three columns, Version, BinaryMode, and NumberOfBits or similar. In a database you would set a key, in this case a primary key and/or index, and your key would be the combination of both Version and BinaryMode. This tells the database that you may only ever have one row with the same values for those fields, and it therefore allows you to know when you run a query, you will never get two rows from one set of values for each.

As New Dictionary(Of Tuple(Of Integer, String), Integer) From

在VB中,这是使用初始化程序创建字典的方法:创建一个New Dictionary(Of T1, T2)然后使用该From关键字告诉它初始化程序列表即将到来.整个初始化程序用大括号括起来,然后每个项目以逗号分隔.Key并.Value为项目分隔.

{VersionAndMode(10, "0001"), 10},

现在,我们的Dictionary中的第一项是一个元组(整数,字符串).您可以创建一个元组要么像New Tuple(Of T1, T2)(Item1Val, Item2Val)或者类似的东西Tuple.Create(Of T1, T2)(Item1Val, Item2Val).在实践中,您通常会使用该.Create方法,因为它有一个非常好的功能:它使用类型推断来确定您实际创建的类型.换句话说,你也可以打电话Tuple.Create(Item1Val, Item2Val),编译器会推断T1和T2为您服务.但是,有一个主要原因是我创建了这个辅助函数:

Public Shared Function _
    VersionAndMode(Version As Integer, Mode As String) _
    As Tuple(Of Integer, String)
        Return Tuple.Create(Of Integer, String)(Version, Mode)
End Function

And that's because Tuple doesn't tell you anything about the data you are containing. I might even be tempted in a production application to even create a VersionAndMode class that simply Inherits Tuple<Of Integer, String) just because it's a lot more descriptive.

That pretty much covers the lookup initialization. But what about the actual lookup? Well, let's ignore for a moment where the values are coming from, but so, now the lookup is trivial. The complexity of the If statement in the original is now contained in what I believe is a much more descriptive fashion, it's a declarative way of stating the same information in that procedure. And with that out of the way, we can just focus on what we're doing instead of how we're doing it: Dim NumberOfBits = BitsLookup.Item(Version, Mode). Well, I do declare. :)

还有另一个查找,EncodingModes查找,还有更多要说的内容,所以我将在下一节中介绍它.

方法

一旦我们进行了查找,我们就可以看看其他方法.这是我实施的.

比特数查找

那么,这就是大If集团的剩余部分:

Public ReadOnly Property NumberOfBits As Integer
    Get
        Return BitsLookup.Item(
            VersionAndMode(Version, BinaryMode)
        )
    End Get
End Property

关于这一点并没有什么可说的.

表格初始化方法

When you have a nice designer, it's tempting to try to do everything there so there's no code to write. However, in our case all the data we need is already contained in the lookups we created. If we were to simply enter the items into the listbox as strings, we'd end up not only repeating ourselves, violating one of the general principles of development (DRY, Don't Repeat Yourself), but we're also losing the nice connections we already have set up with our data.

So, let's take a look at that last lookup:

Public Shared EncodingModes As New Dictionary(Of String, String) From
{
    {"0000", "<Auto Select>"},
    {"0001", "Numeric (max. 7089 chars)"},
    {"0010", "Alphanumeric (max. 4296 chars)"},
    {"0100", "Binary [8 bits] (max. 2953 chars)"},
    {"1000", "Kanji/Kana (max. 1817 chars)"}
}

Here, again, we just have a declarative way of saying the same thing that was said in the method that created the data imperatively, and again, it's advantageous because we only need to create it one time, and after that look data up based on the key. Here our Key is the "Encoding Mode" and the Value holds the text to displayed for any particular Key. But, so, what happens if we just enter the text into our ListBox in the forms designer?

Well, two things. First, we have entered it twice now. Second, is now we either would have to create a different lookup to go back, or, we have to go against the grain in the way a Dictionary is used. It's not impossible, but as you can see in the original get_binary_mode function, it's not very clean either. Plus, we've lost the advantages of the declarative nature of the Dictionary.

So, how do we use the existing lookup to create our ListBox items without repeating ourselves? Well, one thought would be to just grab the Values and put them in a list, and then putting that in the .Items field on the ListBox. But, see, we didn't solve the other problem; still we have to go backward, from Value to Key (which in a Dictionary isn't even guaranteed to be unique).

Fortunately, there's a solution: using ListBox.DataSource. This allows us to take many different data structures, and feed them to the listbox (nom nom), rather than being limited to List<T> and things that implement IList. But this doesn't necessarily select the proper items for display, so what do we do if it displays the wrong property? Well, the final missing piece is ListBox.DisplayMember where we set the name of the property to be used for display.

So, here's the code we can use to set up our listboxes:

Private Sub Form1_Load(sender As System.Object, e As System.EventArgs) Handles MyBase.Load
    listboxVersion.DataSource =
        BitsLookup.Keys.Select(
            Function(VAM As Tuple(Of Integer, String)) _
                VAM.Item1()
        ).
        Distinct().ToList()

    If listboxVersion.Items.Count > 0 _
        Then listboxVersion.SelectedIndex = 0

    listboxMode.DisplayMember = "Item1"
    listboxMode.DataSource =
        EncodingModes.AsQueryable().Select(
            Function(KVP As KeyValuePair(Of String, String)) _
                Tuple.Create(KVP.Key, KVP.Value)
        ).
        ToList()

    If listboxMode.Items.Count > 0 _
        Then listboxMode.SelectedIndex = 0
End Sub

So, I'm using functionality from LINQ here to get my data in whatever form makes sense from the lookups, setting that as the .DataSource, and then telling the ListBox which member to display. I love it when I get to tell things what to do. :) Now, I can't possibly do justice to Lambda Epxressions here, but let me take a quick stab. So, the first listbox is set up like so:

    listboxVersion.DataSource =
        BitsLookup.Keys.Select(
            Function(VAM As Tuple(Of Integer, String)) _
                VAM.Item1()
        ).
        Distinct().ToList()

Each individual part is fairly understandable, and if you've worked with SQL or other types of queries, I'm sure this doesn't seem too unfamiliar. But, so, the problem with the way we have our data stored right now is that we only have the versions numbers that are in the Tuples in the BitLookup. Worse yet, there are several keys in the lookup that have each value contained in them. [Note that this is likely a sign that we should have that information's primary store somewhere else; it's ok for it to be part of something else, but we really shouldn't usually have data stored such that the primary store of the data contains duplicated information.]

As a reminder of what one of the rows looks like:

    {VersionAndMode(10, "0001"), 10},

So, there are two things we have to accomplish here. Since the UI representation is the same as the actual number here, we don't have to worry about making something other than a list to hold the data. First, we need to figure out how to extract the values from the Keys of that lookup, and second, we need to figure a way to make sure that we don't have multiple copies of the data in our list.

Let's think about how we would do this if we were doing it imperatively. We'd say, ok, computer, we need to look through all the keys in the lookup. (ForEach). Then, we'd look at each one in turn, and take Item1's value (that's the property storing the version number), then probably check to see if it already existed in the list, and finally, if it was not already there (.IndexOf(item) < 0) we would add it. And this would be okay! The most important thing is that this gives the right behavior, and it is quite understandable.

However, it does take up space, and it's still very much concerned with how it's getting done. This would eliminate, for instance, improving performance without mucking about with the procedure itself. Ideally, we would want to be able to just tell the computer what to do, and have it hand it to us on a jewel-encrusted gold platter. (That's better than a silver one any day, right?) And this is where LINQ and Lambda expressions come in.

So, let's look at that code again:

    listboxVersion.DataSource =
        BitsLookup.Keys.Select(
            Function(VAM As Tuple(Of Integer, String)) _
                VAM.Item1()
        ).
        Distinct().ToList()

We're using one of the LINQ extension methods .Select on the Key collection of the lookup, which does about what it sounds like: it selects something based on each item in the Key collection, and puts it all together into a nice collection for us. We're also using the .Distinct() extension on the result, which ensures that there's no more than one of each item in the list, and finally, we're using the ToList() method which puts everything into a list.

Inside the select is where the Lambda Expression comes in:

            Function(VAM As Tuple(Of Integer, String)) _
                VAM.Item1()

Caveat: VB only supports Lambda Expressions for things like this, not Lambda Statements. The difference is that a Lambda Expression does not specify a return type, and does not have an End Function. You'll notice I used a space, underscore pattern at the end of the first line, this is because Lambda Expressions must all be on one line, and the " _" tells the compiler to consider the next line to be continued as if it were one line. For full details on the restrictions, see Lambda Expressions (Visual Basic).

The parentheses on VAM.Item1() were inserted there for me by VB, but they are not required. But this function is what tells the .Select method which item to put into the new collection for each item in the source collection, and it also tells it what type should be collected (in this case an Integer). The default collection type for most of the common LINQ functions, including Select in this case is IEnumerable(T1), and in this case, since we are returning an Integer, the compiler can infer the type of the resulting collection, an IEnumerable(Integer). Distinct() remove duplicates and also returns IEnumerable(Integer), and ToList() returns a List(Integer) from an IEnumerable(Integer).

And that's type we need to set for our ListBox, so we're done with that!

And, also, there's the listbox with the Encoding Mode:

    listboxMode.DataSource =
        EncodingModes.AsQueryable().Select(
            Function(KVP As KeyValuePair(Of String, String)) _
                Tuple.Create(KVP.Key, KVP.Value)
        ).
        ToList()

This code works the very same way: we take the EncodingModes lookup Dictionary with items like {"0000", "<Auto Select>"},, we perform a Select to get an IEnumerable returned to us, the function takes a single line (KeyValuePair) from the dictionary, but then it does something a little different. It returns a Tuple with the Key and Value both! Why becomes apparent in the final section, but the important thing is that we're returning something that has both the pieces of data in it, and this is in fact the solution to the problem with figuring out how to get the data we need from the listbox.

So, we're in the home stretch. Here are the last couple of items we use to set the textbox with the number of bits:

Private ReadOnly Property Version As Integer
    Get
        Dim SelectedVersion As Integer = _
            listboxVersion.SelectedItem

        Return SelectedVersion
    End Get
End Property

This property just returns the current value from the ListBox, which contains the values we pulled out of the lookup in the setup.

Private ReadOnly Property BinaryMode As String
    Get
        Dim EncodingMode As Tuple(Of String, String) = _
            listboxMode.SelectedItem

        Dim RetVal As String = "0001"

        If EncodingMode.Item1 <> "0000" _
            Then RetVal = EncodingMode.Item1

        Return RetVal
    End Get
End Property

And this property pulls the BinaryMode, but notice: there's no need to use the Dictionary in reverse: since we used a DataSource, we can simply pull out the selected item, cast it to the data type we put in, and then we can get out the associated bit of data without ever having to go back to the Dictionary.

Just by the fact that the user selected a particular item, we know what the corresponding binary key is, and return that. And, the other cool thing about that is that even if there were duplicate Values in the Dictionary, there would be no ambiguity about which was the proper value. (Now, the user wouldn't know, and that's a problem, but can't solve everything at once. :D)

The one little hitch in that property was what to do if the EncodingMode turned out to be '0000'. (That had a value of "<Auto Select>" in the Values, and is not accounted for by the lookup.) So, I auto selected it to be "0001"! I'm sure a more intelligent manner would be chosen for a real application, but that's good enough for me, for now.

Pulling (Putting?) It All Together

Well, the very last piece of the puzzle, the thing that actually gets the number of bits and sets it to the TextBox on the form:

Private Sub btnSelect_Click(sender As System.Object, e As System.EventArgs) _
      Handles btnSelect.Click
    txtNumberOfBits.Text = NumberOfBits.ToString()
End Sub

So, all we had to do is take the NumberOfBits field which returns the number of bits based on the items the user has selected for Version and EncodingMode. Kinda anti-climactic, huh?

Well, sorry for the length, I hope this has been helpful, I know I learned a few things. :)