0

I am trying to make a simple Temp converter to be able to correctly switch between Fahrenheit, Celsius, and Kelvin. The code isn't working the way it's supposed to. When I go from Fahrenheit to Kelvin it tells me it's using code to go from Fahrenheit to Celsius not Kelvin. I even added msgbox to tell me what is pulling what. I have also added the options for my combo boxes. It likes to ignore everything when using " " as a string. I have tried using both & and And to get this to work.

Private Sub CommandButton10_Click()
    If ComboBox1.Value = Fahrenheit & ComboBox2.Value = Celsius Then    
        Call FtoC        
    ElseIf ComboBox1.Value = Celsius & ComboBox2.Value = Fahrenheit Then
        Call CtoF        
    ElseIf ComboBox1.Value = Celsius & ComboBox2.Value = Kelvin Then    
        Call CtoK        
    ElseIf ComboBox1.Value = Kelvin & ComboBox2.Value = Celsius Then        
        Call KtoC        
    ElseIf ComboBox1.Value = Fahrenheit & ComboBox2.Value = Kelvin Then    
        Call FtoK    
    ElseIf ComboBox1.Value = Kelvin & ComboBox2.Value = Fahrenheit Then
        Call KtoF
    End If
End Sub

This is the code for initialize for my comboboxes 

Private Sub UserForm_Initialize()
    Call CallSetting

    'change position
    Me.StartUpPosition = 0
    Me.Top = (Application.Height / 2) - Me.Height / 2
    Me.Left = (Application.Width / 2) - Me.Width / 2

    With ComboBox1
        .AddItem ("Fahrenheit")
        .AddItem ("Celsius")
        .AddItem ("Kelvin")
    End With

    With ComboBox2
        .AddItem ("Fahrenheit")
        .AddItem ("Celsius")
        .AddItem ("Kelvin")
    End With

    TextBox2.Locked = True
End Sub

My macros are defined as the following: 

Sub FtoC()
    TempConverter.TextBox2.Value = (TempConverter.Temp1.Value - 32) * (5 / 9)  
    MsgBox "I am FtoC"
End Sub

Sub CtoF()
    TempConverter.TextBox2.Value = (TempConverter.Temp1.Value * 0.55555) + 32
    MsgBox "I am CtoF"
End Sub

Sub CtoK()
    TempConverter.TextBox2.Value = TempConverter.Temp1.Value + 273  
    MsgBox "I am CtoK"
End Sub

Sub KtoC()
    TempConverter.TextBox2.Value = TempConverter.Temp1.Value - 273   
    MsgBox "I am KtoC"
End Sub

Sub FtoK()
    TempConverter.TextBox2.Value = ((TempConverter.Temp1.Value - 32) * (5 / 9)) + 273
    MsgBox "I am FtoK"
End Sub

Sub KtoF()
    TempConverter.TextBox2.Value = ((TempConverter.Temp1.Value - 273) * (5 / 9)) + 32    
    MsgBox "I am KtoF"
End Sub
Improve this question
4
  • 6
    change & to the actual word And. Ampersand is not recognizable syntax for logic operators. Commented Jun 14, 2018 at 13:00
  • 4
    In ComboBox1.Value = Fahrenheit is Fahrenheit a variable? then declare it properly. If it is a string then use ComboBox1.Value = "Fahrenheit". I recommend to use Option Explicit. Commented Jun 14, 2018 at 13:01
  • 3
    further to @Pᴇʜ point, using Option Explicit at the top of each code module will ensure you have proper sytnax and variable declaration in your code (and save you a lot of troubleshooting). To set this automatically, in the VBE, go to Tools > Options > Editor > Require Variable Declarations. Commented Jun 14, 2018 at 13:02
  • Additionally I recommend to rename your ComboBoxes. Names like ComboBox1 are completely meaningless and hard to understand. A better practice would be to name them meaningful like cmbFromUnit and cmbToUnit. If you use meaningful object and variable names you make life much easier for yourself (especially as a beginner). • Apply all these comments here and your issue is solved. Commented Jun 14, 2018 at 14:29

3 Answers 3

Reset to default
4

you could avoid all that If Then ElseIf ... using CallByName method

Private Sub CommandButton10_Click()
    CallByName Me, ComboBox1.Value & "To" & ComboBox2.Value, VbMethod
End Sub

just name properly (and more meaningfully...) your routines:

Sub FahrenheitToCelsius()
    TempConverter.TextBox2.Value = (TempConverter.Temp1.Value - 32) * (5 / 9)
    MsgBox "I am FtoC"
End Sub

Sub CelsiusToFahrenheit()
    TempConverter.TextBox2.Value = (TempConverter.Temp1.Value * 0.55555) + 32
    MsgBox "I am CtoF"
End Sub

Sub CelsiusToKelvin()
    TempConverter.TextBox2.Value = TempConverter.Temp1.Value + 273
    MsgBox "I am CtoK"
End Sub

Sub KelvinToCelsius()
    TempConverter.TextBox2.Value = TempConverter.Temp1.Value - 273
    MsgBox "I am KtoC"
End Sub

Sub FahrenheitToKelvin()
    TempConverter.TextBox2.Value = ((TempConverter.Temp1.Value - 32) * (5 / 9)) + 273
    MsgBox "I am FtoK"
End Sub

Sub KelvinToFarenheit()
    TempConverter.TextBox2.Value = ((TempConverter.Temp1.Value - 273) * (5 / 9)) + 32
    MsgBox "I am KtoF"
End Sub
Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

I would even convert the procedures into functions for a even better re-usability of them.
OMG, I've tried everything you have told me to try and to fix. NOTHING, what in the world is going on?
I would even make a single conversion function that takes three parameters: the value, the original unit, the intended unit. And then run the correct formula based on the combination of original/intended unit. This would be easier to maintain and extend in the future. And also avoid confusing CallByName constructs.
@AJD I like the concept of: "One function does one thing only and does that right" instead of "One function does multiple things, and gets complicated". Sometimes it is better to write a little more code and keep that simple. It can make it easier to maintain and more human readable (this means less errors). I see nothing wrong having one function for one conversion type. I even would consider this is a good practice.
@Pᴇʜ: I agree, but where the interesting discussion starts is defining "one thing". I would say "one thing" here is converting a temperature value from one set of units to another. OP's example above has broken the "one thing" even further basically defining one line of code as "one thing" (which is taking it to the extreme). For me, the function is one input (value + unit) and one output (value in context of new unit) - using three parameters to achieve this result.
1

Based on your replies, I assume that you did not have Option Explicit included in your module.

As such, your terms Farenheit, Celcius and Kelvin where implicitly defined and would have taken a default value (e.g. "" or 0). So your comparisons were not comparing what you thought they were.

When you put the "" around the words, you explicitly defined them as constant strings. So now your code could cleanly compare what was in the ComboBox with your options.

To cover off some of my comments, you can address your calculations in a single function. Here is an example:

Function ConvertTemperatureUnit(TemperatureValue as Double, FromUnits as String, ToUnits as String) as Double
    Select Case FromUnits
        Case "Fahrenheit"
            Select Case ToUnits
                Case "Celcius"
                    ConvertTemperatureUnit = (TemperatureValue -32) * (5/9)
                Case "Kelvin"
                    ConvertTemperatureUnit = ((TemperatureValue - 32) * (5 / 9)) + 273.15 ' accuracy!
                Case Else
                    ConvertTemperatureUnit = TemperatureValue ' covers an error case that was not covered in your previous code!
        Case "Celcius"
            Select Case ToUnits
                Case "Fahrenheit"
                    ConvertTemperatureUnit = (TemperatureValue * 5/9) + 32 ' keep the form of formula consist - just a readability thing.
                Case "Kelvin"
                    ConvertTemperatureUnit = TemperatureValue + 273.15
                Case Else
                    ConvertTemperatureUnit = TemperatureValue
        Case "Kelvin"
            Select Case ToUnits
                Case "Fahrenheit"
                    ConvertTemperatureUnit = ((TemperatureValue - 273.15) * (5 / 9)) + 32
                Case "Celcius"
                    ConvertTemperatureUnit = TemperatureValue - 273.15
                Case Else
                    ConvertTemperatureUnit = TemperatureValue
        Case Else
            ConvertTemperatureUnit = TemperatureValue ' soft fail.
    End Select

End Function

Thanks to @Peh, here is another example:

Function ConvertTemperatureUnit(TemperatureValue as Double, FromUnits as String, ToUnits as String) as Double
    Select Case FromUnits & "To" & ToUnits  ' the added "To" makes it human readable
        Case "FahrenheitToCelcius"
            ConvertTemperatureUnit = (TemperatureValue -32) * (5/9)
        Case "FahrenheitToKelvin"
            ConvertTemperatureUnit = ((TemperatureValue - 32) * (5 / 9)) + 273.15
        Case "CelciusToFahrenheit"
            ConvertTemperatureUnit = (TemperatureValue * 5/9) + 32 
        Case "CelciusToKelvin"
            ConvertTemperatureUnit = TemperatureValue + 273.15
        Case "KelvinToCelcius"
            ConvertTemperatureUnit = TemperatureValue - 273.15
        Case "KelvinToFahrenheit"
            ConvertTemperatureUnit = ((TemperatureValue - 273.15) * (5 / 9)) + 32
        Case Else
            ConvertTemperatureUnit = TemperatureValue ' soft fail.
    End Select

End Function
Improve this answer

Comments

0

I have tried to add " " around the different temperature scales many times and nothing was working. I am not sure why, but it worked this time. I am still very confused, but it works now. Thank you for all your help.

    If ListFromUnit.Value = "Fahrenheit" And ListToUnit.Value = "Celsius" Then
        Call FtoC
    ElseIf ListFromUnit.Value = "Celsius" And ListToUnit.Value = "Fahrenheit" Then
         Call CtoF
    ElseIf ListFromUnit.Value = "Celsius" And ListToUnit.Value = "Kelvin" Then
         Call CtoK
    ElseIf ListFromUnit.Value = "Kelvin" And ListToUnit.Value = "Celsius" Then
         Call KtoC
    ElseIf ListFromUnit.Value = "Fahrenheit" And ListToUnit.Value = "Kelvin" Then
         Call FtoK
    ElseIf ListFromUnit.Value = "Kelvin" And ListToUnit.Value = "Fahrenheit" Then
         Call KtoF
    End If
Improve this answer

4 Comments

Instead of cascading If-ElseIf, this is a good example where a nested Select Case can be used. This would be easier to maintain as other units are added (OK, what other units - but the idea is extendable to other measurements). But my other comment about a single function applies here as well, and I think the repetitiveness of this sort of code reinforces that idea.
@AJD Select Case can only handle 1 condition but here we have 2 conditions which makes it even more complicate when you use Select I think. At least you would have to combine both conditions to one and have a case for each combination.
@Pᴇʜ: That is why I said nested case, but you are right when it becomes a little more complex. What makes it easier to maintain is that the conditions are more clearly seen rather than the "If ... And ...". But that is probably subjective anyway.
@AJD Well, you could combine it to Select Case ListFromUnit.Value & "To" & ListToUnit.Value and then check with Case "FahrenheitToCelsius" as a workaround to avoid nesting.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.