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 am using the code below to capitalize words in a string. I do not want conjunction words capitalized, but always want the first character in the string to be capitalized no matter what the word is. Is this the most optimized way of accomplishing this? How could this be improved?

xText = queryForHeading    
xTextSplit = split(xText, " ")

for each item in xTextSplit
    xWord = item
    if lcase(item) = "a" or lcase(item) = "an" or lcase(item) = "and" or lcase(item) = "as" or lcase(item) = "at" or lcase(item) = "but" or lcase(item) = "by" or lcase(item) = "en" or lcase(item) = "for" or lcase(item) = "if" or lcase(item) = "in" or lcase(item) = "is" or lcase(item) = "nor" or lcase(item) = "of" or lcase(item) = "on" or lcase(item) = "or" or lcase(item) = "per" or lcase(item) = "the" or lcase(item) = "this" or lcase(item) = "to" or lcase(item) = "it" or lcase(item) = "vs." then
        xWord = lcase(item)
    elseif lcase(item) = "ii" or lcase(item) = "iii" or lcase(item) = "iv" or lcase(item) = "vi" or lcase(item) = "vii" or lcase(item) = "viii" or lcase(item) = "ix" then
        xWord = ucase(item)
    end if
    xCompleteWord = xCompleteWord & " " & xWord
next

queryForHeading = xCompleteWord
queryForHeading = Mid(queryForHeading, 2)
queryForHeading = UCase(Mid(queryForHeading, 1, 1)) & Mid(queryForHeading, 2)
share|improve this question
1  
The last question is off-topic as we don't add additional implementation. If everything else works, then that can be reviewed. –  Jamal May 15 at 17:37
    
Ok ... understood. –  user3633172 May 15 at 17:39

2 Answers 2

up vote 4 down vote accepted

I think that you should be using ArrayLists for those conditional statements.

specifically you should store your conjunction words in an ArrayList something like this

Dim conjunctions
Set conjunctions = CreateObject("System.Collections.Arraylist")

conjunctions.Add("a")
conjunctions.Add("an")
conjunctions.Add("and")
conjunctions.Add("as")
conjunctions.Add("at")
conjunctions.Add("by")
' Etc ......

for each item in xTextSplit
    xWord = item
    if conjunctions.contains(lcase(item))
        xWord = lcase(item)
    elseif suffixes.contains(lcase(item))
          xWord = ucase(item)
    end if
    xCompleteWord = xCompleteWord & " " & xWord
next

suffixes would be another ArrayList. this could be used with the method that Vogel612 suggests.

creating an ArrayList is a better way to perform this task because now you can add more words to the conjunction ArrayList and not have to worry if you found all the instances of it in the code. It is more Maintainable.

NOTE: I haven't tested this code


I think that you should be using Dictionary's for those conditional statements.

specifically you should store your conjunction words in a dictionary something like this

Dim conjunctions   ' Create a variable.
Set conjunctions = CreateObject("Scripting.Dictionary")

conjunctions.Add(1,"a")
conjunctions.Add(2,"an")
conjunctions.Add(3,"and")
conjunctions.Add(4, "as")
conjunctions.Add(5, "at")
conjunctions.Add(6, "by")
' Etc ......

for each item in xTextSplit
    xWord = item
    if conjunctions.Exist(lcase(item))
        xWord = lcase(item)
    elseif suffixes.Exist(lcase(item))
          xWord = ucase(item)
    end if
    xCompleteWord = xCompleteWord & " " & xWord
next

suffixes would be another dictionary. this could be used with the method that Vogel612 suggests.

share|improve this answer

You could drastically shorten your If statements by extracting them to a function. Furthermore I nowhere see, that you actually change the case for xWord. That also makes the assignment at the start of your loop useless. Your code then changes to:

for each item in xTextSplit
    if isConjunction(item) then
        xWord = lcase(item)
    elseif isRomanNumeral(item) then
        xWord = ucase(item)
    else
        xWord = ucaseFirst(item)
    end if
    completeWord = completeWord & " " & xWord
next

'your sub ends here

private function isConjunction (String item) as Boolean
   isConjunction = 'your if-statement here
end function

private function isRomanNumeral (String item) as Boolean
   isRomanNumeral = 'the roman numbers if-statement here
end function

private function ucaseFirst(String item) as Variant/String
     ucaseFirst = ucase(left(item, 1)) & Mid(item, 2)
end function
share|improve this answer
    
I thought you were going to take my idea. which you sort of did but at the same time you didn't. –  Malachi May 15 at 17:57

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.