Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I wrote an algorithm which should cut a companies name into 3 strings.

Input: 1 String

Output: 3 String.

Conditions:

String 1 2 and 3 shall not be longer then 35 signs. If the Input string is longer then it should be cut to a length of 105.

If you have fun and be interested in algorithms it would be nice if you take a look at it.

   Public Sub CompanyCut()
  //3 output Strings
    Dim var1 As String = ""
    Dim var2 As String = ""
    Dim var3 As String = ""



    If Module1.insertdict("company").Length > 35 Then



        Dim s1 As String = Module1.insertdict("company").ToString

        If s1.Length > 105 Then
            s1 = Microsoft.VisualBasic.Left(s1, 105)
        End If

        //Split String into array at every Whitespace
        Dim pattern As String = "\s"
        Dim sa() As String = Regex.Split(s1, pattern)


        //Variables for loop
        Dim i As Integer = 0
        Dim varyn As Boolean = True
        Dim varyn1 As Boolean = False
        Dim varyn2 As Boolean = False



        //loop which fills var1 var2 and var3 with arrayfields untill size 35
           would be reached

        For i = 0 To sa.Length - 1

            If var1.Length < 35 AndAlso varyn = True Then
                If var1.Length + 1 + sa(i).Length < 35 Then
                    var1 = var1 + " " + sa(i).ToString
                Else
                    varyn = False
                    varyn1 = True
                    varyn2 = False

                End If

            End If

            If var2.Length < 35 AndAlso varyn1 = True Then
                If var2.Length + 1 + sa(i).Length < 35 Then
                    var2 = var2 + " " + sa(i).ToString
                Else
                    varyn1 = False
                    varyn = False
                    varyn2 = True

                End If

            End If

            If var3.Length < 35 AndAlso varyn2 = True Then
                If var3.Length + 1 + sa(i).Length < 35 Then
                    var3 = var3 + " " + sa(i).ToString
                Else
                    varyn2 = False

                End If

            End If




        Next

        //my idea was that if it has the same or bigger length all fields must
          be in + the whitespaces

            If var1.Length + var2.Length + var3.Length >= s1.Length Then

            Module1.insertdict("Firma") = var1
            Module1.insertdict("Name2") = var2
            Module1.insertdict("Name3") = var3



        Else

            //this occurs when the string is smaller as 105 signs but not all 
              fields  of the array could be placed in the variables.

            Module1.insertdict("Failure") = "Company name need to split by user"




        End If

    Else


        Module1.insertdict("Name2") = ""
        Module1.insertdict("Name3") = ""

    End If

End Sub

**Edit: I forgot to bring up in the Conditions that if it is possible it should be cut at >the whitespace.

What to do now? shall I open a new Post?**

My main problem is to find a sub algorithmn which handles this case

Else

        //this occurs when the string is smaller as 105 signs but not all 
          fields  of the array could be placed in the variables.

        Module1.insertdict("Failure") = "Company name need to split by user"




End If

An easy way would be to cut it with substring like in the answers. a more elegant way would be to cut the first 35 chars for the first variable and then trying to cut at whitespaces. If it doesnt works also cut the second 35 for var2. and the rest is going to var3 :)

share|improve this question
    
Your solution attempts to split at whitespace. This is a nice idea, but not explicit in the requirement (and causes many problematic special cases). (Actually, the requirement that the concatenation of the output strings is the input string is also missing from the problem statement) –  Hagen von Eitzen Jul 3 at 15:13
1  
VB uses apostrophes (') to begin single-line comments, not // –  Pierre Menard Jul 3 at 18:10
1  
Check out word wrap algorithms. This is a commonly solved problem in text editor. –  Lie Ryan Jul 4 at 10:06

4 Answers 4

Just for the fun of it, here's a one-liner using LINQ.

Dim names = (From i In {0, 35, 70} Where i < input.Length Select input.Substring(i, Math.Min(35, (input.Length - i))))

Example

Dim names As String() = (From i In {0, 35, 70} Where i < input.Length Select input.Substring(i, Math.Min(35, (input.Length - i)))).ToArray()
Dim var1 As String = If((names.Length > 0), names(0), String.Empty)
Dim var2 As String = If((names.Length > 1), names(1), String.Empty)
Dim var3 As String = If((names.Length > 2), names(2), String.Empty)
share|improve this answer
3  
+1 for LINQification. –  Konrad Viltersten Jul 4 at 6:22

You don't need to loop. The function SubString and the property Length is enought to handle what you want.

    var1 = companyName
    var2 = ""
    var3 = ""

    If var1.Length > 35 Then
        var2 = var1.Substring(35)
        var1 = var1.Substring(0, 35)

        If var2.Length > 35 Then
            var3 = var2.Substring(35)
            var2 = var2.Substring(0, 35)

            If var3.Length > 35 Then
                var3 = var3.Substring(0, 35)
            End If
        End If
    End If

This could be even smaller by using arrays

    Dim vars(3) As String

    vars(0) = companyName

    For i As Integer = 0 To 2
        If vars(i).Length > 35 Then
            vars(i + 1) = vars(i).Substring(35)
            vars(i) = vars(i).Substring(0, 35)
        Else
            Exit For
        End If
    Next
share|improve this answer
    
you should get rid of the Else statement and the Exit For there is no need for it whatsoever –  Malachi Jul 3 at 15:06
    
@Malachi: No, you do need that. If the company name input string is 70 characters or less, you only need to fill indexes 0 and 1 because there will be no string for the third index. Thus you want to short circuit when i is 2. –  ps2goat Jul 3 at 22:16
    
vars(i + 1) = vars(i).Substring(35) Do I see something wrong, or did you miss to pass var(i+1) the string from (35,endofstring) –  McDauly Jul 4 at 6:51
2  
@McDauly If you look at the documentation for Substring, the function goes to endofstring if only one parameter is passed. –  the_lotus Jul 4 at 12:51

Everything in moderation

Whitespace can help make your code nice and readable. However, TOO much whitespace can make your code hard to read. You fall into the latter case: you have way too much whitespace. A single blank line is all that is necessary to distinguish different logical sections.


DRY - Don't Repeat Yourself

Each iteration through your for loop, you get the length of sa(i) 3 times and could have to convert sa(i) to a string at most 3 times. Instead, do the calculations up front and store the results in a variable.


Be consistent

Keep indents on the same level, otherwise you can confuse both yourself and potential readers later on. Currently you have:

//my idea was that if it has the same or bigger length all fields must
  be in + the whitespaces

    If var1.Length + var2.Length + var3.Length >= s1.Length Then

    Module1.insertdict("Firma") = var1
    Module1.insertdict("Name2") = var2
    Module1.insertdict("Name3") = var3
Else

    //this occurs when the string is smaller as 105 signs but not all 
      fields  of the array could be placed in the variables.

    Module1.insertdict("Failure") = "Company name need to split by user"
End If

This is confusing: does the Else statement go with another if statement? Do all the Module1.insert... statements belong inside the if statement or in another outside block of code?

Instead, indent consistently which gets this code:

//my idea was that if it has the same or bigger length all fields must
//be in + the whitespaces
If var1.Length + var2.Length + var3.Length >= s1.Length Then

    Module1.insertdict("Firma") = var1
    Module1.insertdict("Name2") = var2
    Module1.insertdict("Name3") = var3
Else

    //this occurs when the string is smaller as 105 signs but not all 
    //fields  of the array could be placed in the variables.

    Module1.insertdict("Failure") = "Company name need to split by user"
End If

This single indentation change removed all of the confusion

share|improve this answer
    
Sorry I my fail, I never considered that the code is read on "levels". I will do it next time. –  McDauly Jul 4 at 6:53

I like the_lotus first block of code, but I think you would want to remove the nested if statements

var1 = companyName
var2 = ""
var3 = ""

If var1.Length > 105 Then
    var3 = var1.Substring(70,105)
    var2 = var1.Substring(35,70)
    var1 = var1.Substring(0,35)
ElseIf var1.Length > 70 Then
    var3 = var1.Substring(70)
    var2 = var1.Substring(35,70)
    var1 = var1.Substring(0,35)
ElseIf var1.Length > 35 Then
    var2 = var1.Substring(35)
    var1 = var1.Substring(0,35)
End If

anything other than these cases will just be var1

So there is no need for nested if statements.


I was looking at the fancy for loop from the_lotus's answer, and I am thinking that my single if block is going to be more efficient and easier to read.

The reason I am saying this, is that there isn't much that is dynamic about this application, it is pretty set in simple rules.

  • 3 variables
    • 35 characters long (each)

Just no need for anything more complex than this.

Although the for loop is shorter (code character wise) it is doing a lot more than this simple if statement.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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