Temperature converter using "switch" statement

Question

I wrote a program to convert temperature into different units, where the users have to put a space between temperature and unit:

23 f, 34 c, 45 k

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace PracticeSet1
{
 class Program
 {
 public static float temp;
 public static char tempUnit;
 static void Main(string[] args)
 {
 //Getting user input 
 Console.WriteLine("Enter Temprature in to convert it into other i.e 30 k, 45 f, 50 c *Put space between value and unit* ");
 string[] tempInput = Console.ReadLine().Split();
 //parse element 0
 temp = float.Parse(tempInput[0]);
 //assinging tempUnit 
 tempUnit = char.Parse(tempInput[1]);
 switch (tempUnit)
 {
 //Converting temp to F and K if tempUnit == c
 case 'c':
 Console.WriteLine("Celsius To Farhenheit and Kelvin");
 convertCelsiusToFarhenheit();
 convertCelsiusToKelvin();
 break;
 //Converting temp to C and F if tempUnit == K
 case 'k':
 Console.WriteLine("Kelvin");
 convertKelvinToCelsius();
 convertKelvinToFarhenheit();
 break;
 //Converting temp to C and K if tempUnit == F
 case 'f':
 Console.WriteLine("Farhenheit to Celsius and kelvin");
 convertFarhenheitToCelsius();
 convertFarhenheitToKelvin();
 break;
 }
 }
 static void convertFarhenheitToCelsius()
 {
 Console.WriteLine((temp - 32) * 0.5556 + "°C");
 }
 static void convertFarhenheitToKelvin()
 {
 Console.WriteLine((temp + 459.67) * 5 / 9 + "°K");
 }
 static void convertCelsiusToFarhenheit()
 {
 Console.WriteLine((temp * 1.8) + 32 + "°F");
 }
 static void convertCelsiusToKelvin()
 {
 Console.WriteLine(temp + 273.15 + "°K");
 }
 static void convertKelvinToCelsius()
 {
 Console.WriteLine(temp - 273.15 + "°C");
 }
 static void convertKelvinToFarhenheit()
 {
 Console.WriteLine(temp - 459.67 + "°F");
 }
 }
}

• t3chb0t: How about this? If you don't agree with my edit, you can revert it, but I find it's better that way, without requesting any concrete features - if anyone suggests them anyway, it's up to them but it's not ok to ask Code Review to exend your code in a particular way - this makes it off-topic. I'm turning my vote to your favor ;-)
• oerkelens: Not so much codereview, but I expect there's a bug in convertKelvinToFarhenheit().
• Kartik: @oerkelens yes and i fixed it thanks for pointing out
• Kartik: can any one tell me what is that bountry ?
• Nikita B: @Kartik as stated in the description: "One or more of the answers is exemplary and worthy of an additional bounty." This bounty is to award relatively low-rep user for (in my opinion) exemplary answer. I still have to wait 24h grace period though before I can do it, sorry about that. :)

Answer 1

public static float temp;
public static char tempUnit;

There's no reason to make these into global variables. It's better to use local variables:

static void Main(string[] args)
{
 float temp;
 char tempUnit;
}

---

If you want a first step into OOP, try wrapping the temp and tempUnit into a class (or struct) of their own.

---

While Console.ReadLine().Split() is essentially the same as Console.ReadLine().Split(' '), I would suggest putting the character in there.

It's a matter of readability. Initially, I assumed it would split on newlines, not spaces, until I remembered. Or, alternatively, add a comments that mentions you're splitting on spaces.

---

You're not handling garbage values. What happens if the user enters 123f or batman or k 123? Try to handle invalid data and show the user an error message that describes the problem.

if(!float.TryParse(tempInput[0], out temp)
{
 Console.WriteLine($"Cannot parse the value \"{tempInput[0]}\"");
 //Wait until user presses enter and close the application
 Console.ReadLine();
 return;
}

The subsequent code is the same, as TryParse will still put a usable value in the temp variable.

---

Console.WriteLine(temp + 273.15 + "°K");

While this currently works, I highly suggest not doing this.

It's very hard to spot that you're first doing a mathematical operation (number + number), and then a string concatenation (number + string, which is handled as string + string).

Suppose you're in a culture where the unit comes before the value (e.g. °K 123). You can't just simply invert the code ("°K" + temp + 273.15); because if temp equals 123, your result will be °K123273.15 because it is now concatenating three string values.
While you could work around this issue with parentheses, it's still not a good fix.

There's no reason to inline all statement. Especially when you're a beginner, but this applies to experts as well. Handling the same operations on multiple lines does not affect application performance, but it does dramatically lower readability.

var convertedValue = temp + 273.15;
var convertedValueString = convertedValue + "°K";

Notice that I didn't use Console.WriteLine() yet. You've had to copy paste that in every method, which is not a reusable pattern.

It would be better to have the method return a string:

static string convertCelsiusToKelvin()
{
 var convertedValue = temp + 273.15;
 var convertedValueString = convertedValue + "°K";
 return convertedValueString;
}

And wrap the method call in the Main() method:

switch (tempUnit)
{
 //Converting temp to F and K if tempUnit == c
 case 'c':
 Console.WriteLine("Celsius To Farhenheit and Kelvin");
 Console.WriteLine(convertCelsiusToFarhenheit());
 Console.WriteLine(convertCelsiusToKelvin());

Note: This can be optimized further, but this is already a good first step. It teaches you to separate the calculation from the presentation of the calculated value.

---

Try to separate the logic into methods more. Your Main() method currently is doing many things that don't really have anything to do with each other:

• Read the values
• Decide which conversion calculations to do
• (As per my suggestion) Printing the converted values.

A minor redesign to show you what I mean. First, let's define a struct for our values:

public struct Temperature
{
 float Value;
 char Unit;
}

You could redesign your main method to be much more abstracted:

static void Main(string[] args)
{
 Temperature input = GetInputFromUser();
 Temperature valueKelvin = CalculateKelvin(input);
 Print(valueKelvin);
 Temperature valueFahrenheit = CalculateFahrenheit(input);
 Print(valueFahrenheit);
 Temperature valueCelsius = CalculateCelsius(input);
 Print(valueCelsius);
 Console.ReadLine();
}

Some explanatory examples:

public static Temperature CalculateKelvin(Temperature inputValue)
{
 switch(inputValue.Unit)
 {
 case 'K':
 //Same unit, so no conversion needed
 return value;
 case 'F':
 // F => K
 return convertFahrenheitToKelvin(inputValue);
 case 'C':
 // C => K
 return convertCelsiusToKelvin(inputValue);
 }
}
public static void Print(Temperature value)
{
 Console.WriteLine($"{value.Value} °{value.Unit}");
}

• Pieter Witvoet: +1, but a convert* method that returns a string instead of a number is still mixing calculation with (re)presentation - it cannot be used as part of another calculation, and the result cannot (easily) be presented in a different format.
• Flater: @PieterWitvoet: Read on to the last paragraph :) I avoided stacking all changes in a single example for clarity's sake. I also mentioned my earlier example as a first step specifically to avoid implying that this was the best possible approach. It was merely a simple explanation of how such a change can easily be introduced. My focus wasn't so much on the format of the temperature value, but rather the use of Console.WriteLine (which renders the method unusable for doing calculations that are not printed to the console).
• Pieter Witvoet: Yeah, the last example is better still, but I just wanted to point out that problem explicitly for the OP's sake. :)
• Shane: Or, alternatively, add a comments that mentions you're splitting on spaces. This is the alternative option, but if anyone is wondering which to choose, I think most would agree that making the code clear is unambiguously better than being 'clever' then adding comments.

Answer 2

I don't like having 4 lines of space. 1-2 lines creates space.

A lot of the same feedback at Flater. +1

Not consistent in capitalization. Input is lower case and output is upper case.

Should accept upper and lower case input and allow for no space.

Need to validate the input and give feedback on what was wrong and let them correct.

Why not handle Rankine? If you going to handle f, c, and k then what is the logic for not handling r?

Provide a key to quit like q.

Use a struct as suggested by Flatter. Need to have a number (not string) for unit testing. I would extend the sruct and do all the conversion there. It is more testable and more portable.

public struct Temperature
{
 const decimal FahrenheitToRankine = 459.67M;
 const string decimalFormat = "F2";
 public enum Unit { F, R, C, K };
 public Unit InputUnit { get; }
 public Decimal InputValue { get; }
 public Decimal ValueFahrenheit { get; }
 public string Fahrenheit { get { return $"{ValueFahrenheit.ToString(decimalFormat)} {Unit.F}"; } }
 public Decimal ValueRankine { get; }
 public string Rankine { get { return $"{ValueRankine.ToString(decimalFormat)} {Unit.R}"; } }
 public override string ToString()
 {
 return $"input {InputValue.ToString(decimalFormat)} {InputUnit}\r\n{Fahrenheit}\r\n{Rankine}";
 }
 public Temperature(decimal value, Unit unit)
 {
 InputValue = value;
 InputUnit = unit;
 if (unit == Unit.F)
 {
 ValueFahrenheit = value;
 ValueRankine = value + FahrenheitToRankine;
 }
 else if (unit == Unit.R)
 {
 ValueRankine = value;
 ValueFahrenheit = value - FahrenheitToRankine;
 }
 else
 {
 ValueFahrenheit = 0;
 ValueRankine = 0;
 throw new IndexOutOfRangeException();
 }
 }
 public Temperature(decimal value, char cUnit)
 : this(value, cToEnum(cUnit)) { }
 public static Unit cToEnum(char cUnit)
 {
 cUnit = char.ToUpper(cUnit);
 Unit eUnit;
 if (cUnit == Unit.F.ToString()[0])
 {
 eUnit = Unit.F;
 }
 else if (cUnit == Unit.R.ToString()[0])
 {
 eUnit = Unit.R;
 }
 else
 {
 eUnit = Unit.F;
 throw new IndexOutOfRangeException();
 }
 return eUnit;
 }
 public Temperature(string sTemp)
 : this(0.0M, Unit.F) { }
 //parse sTemp left as and exersize for the OP
}

Answer 3

I suggest following things

1. Use delegates for calling appropriate conversion methods, and

2. Move the responsibility of conversions to a separate class.

   We can have separate classes for each type of conversion, but for simplicity i have created every thing in same class

Declare a delegate

 public delegate string Convert(double temperature, Func<double, double> logic, string unit

Class "TemperatureConverter" that will do conversions.

 public class TemperatureConverter
 {
 public string PerformConversion (double temperature, Func<double, double> logic, string unit)
 {
 return string.Format("{0} {1}", logic.Invoke(temperature), unit);
 }
 public string FarhenheitUnit
 {
 get
 {
 return "°F";
 }
 }
 public string CelsiusUnit
 {
 get
 {
 return "°C";
 }
 }
 public string KalvinUnit
 {
 get
 {
 return "°K";
 }
 }
 public double FarhenheitToCelsius(double temperature)
 {
 return (temperature - 32) * 0.5556;
 }
 public double FarhenheitToKelvin(double temperature)
 {
 return (temperature + 459.67) * 5 / 9;
 }
 public double CelsiusToFarhenheit(double temperature)
 {
 return (temperature * 1.8) + 32;
 }
 public double CelsiusToKelvin(double temperature)
 {
 return temperature + 273.15;
 }
 public double KelvinToCelsius(double temperature)
 {
 return temperature - 273.15;
 }
 public double KelvinToFarhenheit(double temperature)
 {
 return temperature - 459.67;
 }
 }

Client code is as below. Input can be taken in any format depending on the requirement. I have not added any basic checks.

 class Program
 {
 static void Main(string[] args)
 {
 //Getting user input 
 Console.WriteLine("Enter Temprature in to convert it into other i.e 30 k, 45 f, 50 c *Put space between value and unit* ");
 string[] input = Console.ReadLine().Split();
 var temperature = double.Parse(input[0]);
 var unit = char.Parse(input[1]);
 TemperatureConverter tempConverter = new TemperatureConverter();
 Convert cnv = new Convert(tempConverter.PerformConversion);
 switch (unit)
 {
 case 'c':
 Console.WriteLine("Celsius To Farhenheit and Kelvin");
 Console.WriteLine(cnv.Invoke(temperature, tempConverter.CelsiusToFarhenheit, tempConverter.FarhenheitUnit));
 Console.WriteLine(cnv.Invoke(temperature, tempConverter.CelsiusToKelvin, tempConverter.KalvinUnit));
 break;
 case 'f':
 Console.WriteLine("Farhenheit To Celsius and Kelvin");
 Console.WriteLine(cnv.Invoke(temperature, tempConverter.FarhenheitToCelsius, tempConverter.CelsiusUnit));
 Console.WriteLine(cnv.Invoke(temperature, tempConverter.FarhenheitToKelvin, tempConverter.KalvinUnit));
 break;
 case 'k':
 Console.WriteLine("Kelvin To Celsius and Farhenheit");
 Console.WriteLine(cnv.Invoke(temperature, tempConverter.KelvinToCelsius, tempConverter.CelsiusUnit));
 Console.WriteLine(cnv.Invoke(temperature, tempConverter.KelvinToFarhenheit, tempConverter.FarhenheitUnit));
 break;
 }
 Console.ReadLine();
 }
 }

• Pieter Witvoet: What's the use of a delegate if you can just call tempConverter.PerformConversion directly? And why would you need a TemperatureConverter instance if there are no virtual methods and there's no state that needs to be kept? And why does PerformConversion take a conversion method, rather than an already-converted value (it's more of a Format/Display method anyway)?