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.

Below is my first attempt at programming a Celsius to Fahrenheit converter in C# winforms. I'm looking for tips and advice on improving my style and what I can do to make the code more efficent/proper, or is it good as is? I apologize ahead of time if i've forgotten any details and would be happy to provide more upon request.

namespace TemperatureConverter
{
  public partial class Form1 : Form
  {
    public Form1()
    {
        InitializeComponent();
        // Initalize the handler to allow use of only control/number characters.
        this.txtCelsius.KeyPress += new KeyPressEventHandler(txtCelsius_KeyPress);
        // Limits user's inpute to 5 characters max.
        txtCelsius.MaxLength = 5;
        // Prevents the user from resizing window.
        this.FormBorderStyle = FormBorderStyle.FixedDialog;
    }
    // Main function. Converts the input into Fahrenheit and outputs to the targeted label.
    private void btnConvert_Click(object sender, EventArgs e)
    {
        double C;
        double F;

        C = double.Parse(txtCelsius.Text);
        F = C * 9 / 5 + 32;
        lblFahrenheit.Text = F.ToString();
    }
    // Prevents user from entering anything other than Number and Control characters.
    private void txtCelsius_KeyPress(object sender, KeyPressEventArgs e)
    {
        if (!Char.IsControl(e.KeyChar) && !Char.IsNumber(e.KeyChar))
        {
            e.Handled = true;
        }
    }
  }
}
share|improve this question
3  
In this piece of code it does not really matter because it it so short, but if it were longer you should start avoiding one letter variables and given them proper names like celciusValue, fahrenheitValue and eventArgs. –  Nobody Dec 24 '13 at 9:57
4  
e is a pretty typical name for the eventArgs argument. I think .NET may even default to it under some circumstances. Doesn't mean it isn't worth changing, but it is a convention. –  Bobson Dec 24 '13 at 17:22
add comment

2 Answers

up vote 14 down vote accepted

The main change is separation of logic and User Interface. Firstly the calculation methods should ideally not reside in UI methods like event handlers.

Secondly the calculations should really be in it's own class, (but this application is just small enough that it is not ENTIRELY necessary).


in programming we hate Magic Numbers. If you can, move any numbers littered around your code and name them something useful.

so:

As 9/5 + 32 is equal to 1.8 (+32) and that number represents the scale difference, if we declare that at the top of our code like

double FAHRENEHEIT_SCALE_DIFFERENCE = 1.8;  
double FREEZING_POINT_OF_WATER = 32;

Depending on how much you have covered thus far you should then move the actual conversion to it's own method.

(because if you come back to this code in 12-18months will you be able to remember what it does? What if it quadruples in size with more features?

So:

I would suggest something along the lines of

public double convertCelsiusToFahrenheit(double celsius)
{
  return celsius * FAHRENEHEIT_SCALE_DIFFERENCE + FREEZING_POINT_OF_WATER;
}

Next, you can tidy the button click.

private void btnConvert_Click(object sender, EventArgs e)
{
    double celsius = double.Parse(txtCelsius.Text);
    lblFahrenheit.Text = convertCelsiusToFahrenheit(celsius);
}

Finally the golden change is the separation of the logic from the interface.

Now that you have a simple method that is separate from the actual screen; you could move it, and all similar methods into it's own TemperatureConverter class, or something similar.

share|improve this answer
5  
Your refactoring is broken - check the conversion in the original code (hint: a * b + c is not the same as a * (b + c)) –  ChrisWue Dec 24 '13 at 9:52
    
Good catch, corrected equation. Thanks. –  apieceoffruit Dec 24 '13 at 10:06
1  
I think your constants have unfortunate names, though. FAHRENEHEIT_SCALE_DIFFERENCE should be CELSIUS_TO_FAHRENHEIT_FACTOR (remove the typo in Fahrenheit, include Celsius, and it’s not a difference, but a factor), and FREEZING_POINT_OF_WATER – in most of the world, a constant of that name would be either 0 or 273.15. I think I’d use CELSIUS_TO_FAHRENHEIT_OFFSET. Just my 2 cents, of course. –  Christopher Creutzig Dec 24 '13 at 15:29
1  
You should use decimal.TryParse() instead. What if the user asks to convert five or 50C? –  Bobson Dec 24 '13 at 17:24
1  
SCREAMING_CAPS is not in favor as constant naming convention for C#. They should be PascalCased, e.g. FahrenheitScaleDifference and FreezingPointOfWater. –  Jesse C. Slicer Dec 24 '13 at 17:58
show 4 more comments

Here's how I would encapsulate the logic:

public interface ITemperature
{
    double Temperature
    {
        get;
    }
}

public interface ITemperatureConverter<in T, out U> where T: ITemperature where U: ITemperature
{
    U Convert(T temperature);
}

public abstract class TemperatureBase : ITemperature
{
    private readonly double temperature;

    protected TemperatureBase(double temperature)
    {
        this.temperature = temperature;
    }

    public double Temperature
    {
        get
        {
            return this.temperature;
        }
    }
}

public sealed class Fahrenheit : TemperatureBase
{
    public Fahrenheit(double temperature) : base(temperature)
    {
    }

    public static implicit operator Fahrenheit(double temperature)
    {
        return new Fahrenheit(temperature);
    }

    public override string ToString()
    {
        return this.Temperature + "F";
    }
}

public sealed class Celsius : TemperatureBase
{
    public Celsius(double temperature) : base(temperature)
    {
    }

    public static implicit operator Celsius(double temperature)
    {
        return new Celsius(temperature);
    }

    public override string ToString()
    {
        return this.Temperature + "C";
    }
}

public sealed class FahrenheitToCelsiusConverter : ITemperatureConverter<Fahrenheit, Celsius>
{
    public Celsius Convert(Fahrenheit temperature)
    {
        if (temperature == null)
        {
            throw new ArgumentNullException("temperature");
        }

        return new Celsius((5f / 9f) * (temperature.Temperature - 32f));
    }
}

public sealed class CelsiusToFahrenheitConverter : ITemperatureConverter<Celsius, Fahrenheit>
{
    public Fahrenheit Convert(Celsius temperature)
    {
        if (temperature == null)
        {
            throw new ArgumentNullException("temperature");
        }

        return new Fahrenheit(temperature.Temperature * (9f / 5f) + 32f);
    }
}

Your UI code then becomes:

private readonly CelsiusToFahrenheitConverter celsiusToFahrenheitConverter = new CelsiusToFahrenheitConverter();

private void btnConvert_Click(object sender, EventArgs e)
{
    lblFahrenheit.Text = celsiusToFahrenheitConverter.Convert(double.Parse(txtCelsius.Text));
}
share|improve this answer
1  
This feels like serious overkill for the given problem-statement, but, if you are going to go this far, why not take it the full-monty, throw in a Kelvin converter, and then you only need to have a MyUnit-Kelvin converter, and you can then ue any unit you want. The Model you suggest here could be the basis of that. –  rolfl Dec 24 '13 at 20:40
    
Couldn't agree more! But the problem at hand only needed C -> F. More can be created pretty easily. –  Jesse C. Slicer Dec 24 '13 at 22:26
add comment

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.