3
\$\begingroup\$

My requirement is to add two binary numbers, say "1001" and "0101" as binary1 and binary2.

Partial Class Default2
    Inherits System.Web.UI.Page
    Dim carry As Boolean = False ' Boolean variable to hold the carry if occured
    Protected Sub Button1_Click(sender As Object, e As System.EventArgs) Handles Button1.Click
        Dim binary1 As String = TextBox1.Text 'First binary number
        Dim binary2 As String = TextBox2.Text 'Second binary number
        Dim result As String = "" 'to store the result
        For i As Integer = Len(binary1 ) - 1 To 0 Step -1
            result = bin_add(getbyte(binary1 , i), getbyte(binary1, i)) & result ' calling function
        Next
        If carry = True Then
            result = "1" & result 'if a carry remains add it to the MSB
        End If
        MsgBox(rslt) 'Display the result
    End Sub

    Public Function bin_add(b1 As Boolean, b2 As Boolean) As String'Function which performs the addition of each single bits form the two inputs which is passed from the calling function
        Dim result As Boolean
        If b1 AndAlso b2 = True Then 'both values are 1/true
            If carry = True Then
                result = True
                carry = True
            Else
                result = False
                carry = True
            End If
        ElseIf b1 = False And b2 = False Then 'Both are 0/false
            If carry = True Then
                result = carry
                carry = False
            Else
                result = False
                carry = False
            End If
        Else
            If carry = True Then
                result = False
                carry = True
            Else
                result = True
                carry = False
            End If
        End If
        If result = True Then
            Return "1" 'return 1 for Boolean true
        Else
            Return "0" 'return 0 for Boolean false
        End If
    End Function

    Private Function getbyte(s As String, ByVal place As Integer) As String'Function for getting each individual letters from the input string. i got it from net.
        If place < Len(s) Then
            place = place + 1
            getbyte = Mid(s, place, 1)
        Else
            getbyte = ""
        End If
   End Function
End Class

Notes:

  • It gives good results for me only if the no. of digits in both numbers are the same.

Question:

How can I improve the code? Especially, how can I reduce the length of code?

\$\endgroup\$
  • 5
    \$\begingroup\$ We don't accept broken code, so I would stick to the first question. But just really quick: prepend the shorter input with enough 0s so that it has the same length (I'm assuming only positive numbers here) \$\endgroup\$ – tim Aug 18 '14 at 9:49
  • 1
    \$\begingroup\$ In addition to @tim, you can use String.PadLeft() to fill the shorter input. Also check the display of the result. If you have this included in your question, we will help you to do the same only cleaner. \$\endgroup\$ – Heslacher Aug 18 '14 at 10:13
  • 4
    \$\begingroup\$ I'll refrain from a review, but are you aware that the And Keyword does bitwise addition? Once the binary representation is converted to an integer, this should be trivial. \$\endgroup\$ – RubberDuck Aug 18 '14 at 10:26
  • \$\begingroup\$ while evaluating this i got correct answer. i need to shorten the code, and make it workable with all inputs \$\endgroup\$ – Little Master Aug 18 '14 at 10:28
  • 4
    \$\begingroup\$ @LOsunG make it workable with all inputs -> that's asking others to implement functionality for you, Therefore it is beyond the scope of code review. i need to shorten the code -> This is OK ,because code review is to improve the code.Even though shortening is not always better. \$\endgroup\$ – bhathiya-perera Aug 18 '14 at 13:45
5
\$\begingroup\$

For now, let's ignore the fact that there are easier ways to add binary numbers. There are other issues with this code.


Inherits System.Web.UI.Page

Why is code that adds binary numbers inheriting from a UI class? There's no need for this. Separate the concerns and create a module for this code instead.


carry is scoped to the class level. This is a symptom of problems in this code.

Protected Sub Button1_Click(sender As Object, e As System.EventArgs) Handles Button1.Click
    Dim binary1 As String = TextBox1.Text 'First binary number
    Dim binary2 As String = TextBox2.Text 'Second binary number
    Dim result As String = "" 'to store the result
    For i As Integer = Len(binary1 ) - 1 To 0 Step -1
        result = bin_add(getbyte(binary1 , i), getbyte(binary1, i)) & result ' calling function
    Next
    If carry = True Then
        result = "1" & result 'if a carry remains add it to the MSB
    End If
    MsgBox(rslt) 'Display the result
End Sub

Getting the values from the UI makes sense, but then you loop over bin_add, which obviously doesn't actually add anything, or you wouldn't need to loop or have a class variable. All of this logic should happen inside of bin_add.

bin_add should take in two strings, handle all of the logic, and return a single string representing the output. While I'm at it, methods should have PascalCased verb-noun names. This method should be called AddBinary and I will refer to it as such for the rest of the review.

As I said earlier, I wouldn't expect a Function that adds binary numbers to take in Boolean values. I would rather it actually take in a byte and overload the method to handle string representation, but I'm lazy and you don't seem to need all that.

Putting it all together, the signature line

Public Function bin_add(b1 As Boolean, b2 As Boolean) As String

Should look like this.

Public Function AddBinary(value1 as String, value2 as String) As String

Implementing this change will be left as an exercise for the reader.

\$\endgroup\$
4
\$\begingroup\$

Once the user input is validated to be ones or zeros in the textboxes the conversion / addition is simple. Using the OP's input parameters:

    'verify user input as a binary digit 
    For Each c As Char In TextBox1.Text
        If Not (c = "0"c OrElse c = "1"c) Then
            Stop 'not a binary digit - error handling needed
            Exit Sub
        End If
    Next

    For Each c As Char In TextBox2.Text
        If Not (c = "0"c OrElse c = "1"c) Then
            Stop 'not a binary digit - error handling needed
            Exit Sub
        End If
    Next

    'http://msdn.microsoft.com/en-us/library/1k20k614%28v=vs.110%29.aspx
    Dim num1 As Integer = Convert.ToInt32(TextBox1.Text, 2)
    Dim num2 As Integer = Convert.ToInt32(TextBox2.Text, 2)

    Dim answer As Integer = num1 + num2

One of the overloads to Convert.ToInt32 accepts a base(radix) to be used in the conversion.

\$\endgroup\$
  • 2
    \$\begingroup\$ You might want to add, which (by the way absolutely cool) feature of .net you're exploiting, including a link to documentation ;) Additionally you could make the code easier to understand by extracting TextBox1.Text and TextBox2.Text to local variables, but that's yours to decide \$\endgroup\$ – Vogel612 Aug 18 '14 at 11:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.