From 8798073e8243caad771e372e9b87ed6271146d97 Mon Sep 17 00:00:00 2001 From: David Gardiner Date: Mon, 29 Feb 2016 20:59:05 +1030 Subject: [PATCH] Address code review comments * Capitalise Percent and Pixel enums and resource keys * Remove commented code * Rewrite PixelPercent constructors to use int.TryParse * Rewrite PixelPercent.CanParse Add tests for PixelPercent --- .../OpenLiveWriter.Localization/StringId.cs | 14 +-- .../OpenLiveWriter.Localization/Strings.resx | 4 +- .../ImageEditing/EditImageSizesDialog.cs | 12 +- .../Tables/ColumnWidthControl.cs | 4 +- .../Tables/HTMLTableColumn.cs | 2 +- .../Tables/PixelPercent.cs | 54 +++++--- .../Tables/RowPropertiesForm.cs | 2 +- .../Tables/TablePropertiesForm.cs | 6 +- .../OpenLiveWriter.Tests.csproj | 1 + .../PostEditor/Tables/PixelPercentTests.cs | 116 ++++++++++++++++++ 10 files changed, 176 insertions(+), 39 deletions(-) create mode 100644 src/managed/OpenLiveWriter.Tests/PostEditor/Tables/PixelPercentTests.cs diff --git a/src/managed/OpenLiveWriter.Localization/StringId.cs b/src/managed/OpenLiveWriter.Localization/StringId.cs index 6f5ae5e0..108071fa 100644 --- a/src/managed/OpenLiveWriter.Localization/StringId.cs +++ b/src/managed/OpenLiveWriter.Localization/StringId.cs @@ -3124,6 +3124,10 @@ namespace OpenLiveWriter.Localization /// PasteSpecialThinnedLabel, /// + /// Percent (unit of measure) + /// + Percent, + /// /// Ping Servers /// PingPrefName, @@ -3132,9 +3136,9 @@ namespace OpenLiveWriter.Localization /// PingPrefUrl, /// - /// pixels + /// Pixels (unit of measure) /// - pixels, + Pixels, /// /// &Map /// @@ -5169,11 +5173,7 @@ namespace OpenLiveWriter.Localization /// /// There was an unexpected error while uploading the video. /// - YouTubeVideoError, - /// - /// percent - /// - percent + YouTubeVideoError } } diff --git a/src/managed/OpenLiveWriter.Localization/Strings.resx b/src/managed/OpenLiveWriter.Localization/Strings.resx index 553d3a91..f520d958 100644 --- a/src/managed/OpenLiveWriter.Localization/Strings.resx +++ b/src/managed/OpenLiveWriter.Localization/Strings.resx @@ -2783,7 +2783,7 @@ Unimplemented Method: {2} &Ping the following web addresses (one web address for each line) - + pixels @@ -4486,7 +4486,7 @@ This might take a while. &Send Error - + percent \ No newline at end of file diff --git a/src/managed/OpenLiveWriter.PostEditor/PostHtmlEditing/ImageEditing/EditImageSizesDialog.cs b/src/managed/OpenLiveWriter.PostEditor/PostHtmlEditing/ImageEditing/EditImageSizesDialog.cs index 38f8fbcc..80432258 100644 --- a/src/managed/OpenLiveWriter.PostEditor/PostHtmlEditing/ImageEditing/EditImageSizesDialog.cs +++ b/src/managed/OpenLiveWriter.PostEditor/PostHtmlEditing/ImageEditing/EditImageSizesDialog.cs @@ -82,16 +82,16 @@ namespace OpenLiveWriter.PostEditor.PostHtmlEditing numericLargeHeight.Enter += new EventHandler(numeric_Enter); numericLargeWidth.Enter += new EventHandler(numeric_Enter); - this.label4.Text = Res.Get(StringId.pixels); - this.label3.Text = Res.Get(StringId.pixels); + this.label4.Text = Res.Get(StringId.Pixels); + this.label3.Text = Res.Get(StringId.Pixels); this.label2.Text = Res.Get(StringId.ImgSBMaximumHeightLabel1); this.label1.Text = Res.Get(StringId.ImgSBMaximumWidthLabel1); - this.label5.Text = Res.Get(StringId.pixels); - this.label6.Text = Res.Get(StringId.pixels); + this.label5.Text = Res.Get(StringId.Pixels); + this.label6.Text = Res.Get(StringId.Pixels); this.label7.Text = Res.Get(StringId.ImgSBMaximumHeightLabel2); this.label8.Text = Res.Get(StringId.ImgSBMaximumWidthLabel2); - this.label9.Text = Res.Get(StringId.pixels); - this.label10.Text = Res.Get(StringId.pixels); + this.label9.Text = Res.Get(StringId.Pixels); + this.label10.Text = Res.Get(StringId.Pixels); this.label11.Text = Res.Get(StringId.ImgSBMaximumHeightLabel3); this.label12.Text = Res.Get(StringId.ImgSBMaximumWidthLabel3); this.buttonCancel.Text = Res.Get(StringId.CancelButton); diff --git a/src/managed/OpenLiveWriter.PostEditor/Tables/ColumnWidthControl.cs b/src/managed/OpenLiveWriter.PostEditor/Tables/ColumnWidthControl.cs index a149c21d..b75caaae 100644 --- a/src/managed/OpenLiveWriter.PostEditor/Tables/ColumnWidthControl.cs +++ b/src/managed/OpenLiveWriter.PostEditor/Tables/ColumnWidthControl.cs @@ -31,8 +31,8 @@ namespace OpenLiveWriter.PostEditor.Tables // This call is required by the Windows.Forms Form Designer. InitializeComponent(); this.labelWidth.Text = Res.Get(StringId.WidthLabel); - this.rbPixels.Text = Res.Get(StringId.pixels); - this.rbPercent.Text = Res.Get(StringId.percent); + this.rbPixels.Text = Res.Get(StringId.Pixels); + this.rbPercent.Text = Res.Get(StringId.Percent); } protected override void OnLoad(EventArgs e) diff --git a/src/managed/OpenLiveWriter.PostEditor/Tables/HTMLTableColumn.cs b/src/managed/OpenLiveWriter.PostEditor/Tables/HTMLTableColumn.cs index 93fabd1b..1e765dd8 100644 --- a/src/managed/OpenLiveWriter.PostEditor/Tables/HTMLTableColumn.cs +++ b/src/managed/OpenLiveWriter.PostEditor/Tables/HTMLTableColumn.cs @@ -40,7 +40,7 @@ namespace OpenLiveWriter.PostEditor.Tables { get { - return new PixelPercent((string) _baseCell.width); //TableHelper.GetCellWidth(_baseCell); + return new PixelPercent((string) _baseCell.width, CultureInfo.CurrentCulture); } set { diff --git a/src/managed/OpenLiveWriter.PostEditor/Tables/PixelPercent.cs b/src/managed/OpenLiveWriter.PostEditor/Tables/PixelPercent.cs index 54a99427..fd15ae0a 100644 --- a/src/managed/OpenLiveWriter.PostEditor/Tables/PixelPercent.cs +++ b/src/managed/OpenLiveWriter.PostEditor/Tables/PixelPercent.cs @@ -31,7 +31,7 @@ namespace OpenLiveWriter.PostEditor.Tables Value = value; } - public PixelPercent(string text, IFormatProvider provider, PixelPercentUnits units) + public PixelPercent(string text, IFormatProvider provider, PixelPercentUnits units) : this() { if (string.IsNullOrEmpty(text)) { @@ -43,9 +43,16 @@ namespace OpenLiveWriter.PostEditor.Tables var s = text.Trim(); Units = units; - var value = int.Parse(s, provider); - - Value = value; + int value; + if (int.TryParse(s, NumberStyles.Integer, provider, out value)) + { + Value = value; + } + else + { + Units = PixelPercentUnits.Undefined; + Value = 0; + } } } @@ -67,28 +74,41 @@ namespace OpenLiveWriter.PostEditor.Tables s = s.TrimEnd('%'); } - var value = int.Parse(s, provider); - Value = value; - Units = units; + int value; + if (int.TryParse(s, NumberStyles.Integer, provider, out value)) + { + Value = value; + Units = units; + } + else + { + Units = PixelPercentUnits.Undefined; + } } } public static bool CanParse(string text) { - try + if (string.IsNullOrEmpty(text)) { - var p = new PixelPercent(text, CultureInfo.InvariantCulture); return true; } - catch (Exception) - { - return false; - } - - } - public PixelPercent(string text) : this(text, CultureInfo.CurrentCulture) - { + var s = text.Trim(); + + if (s.EndsWith("%")) + { + s = s.TrimEnd('%'); + } + + int value; + + if (int.TryParse(s, NumberStyles.Integer, CultureInfo.InvariantCulture, out value)) + { + return true; + } + + return false; } public int Value { get; private set; } diff --git a/src/managed/OpenLiveWriter.PostEditor/Tables/RowPropertiesForm.cs b/src/managed/OpenLiveWriter.PostEditor/Tables/RowPropertiesForm.cs index 756d28fe..d342d880 100644 --- a/src/managed/OpenLiveWriter.PostEditor/Tables/RowPropertiesForm.cs +++ b/src/managed/OpenLiveWriter.PostEditor/Tables/RowPropertiesForm.cs @@ -40,7 +40,7 @@ namespace OpenLiveWriter.PostEditor.Tables this.buttonOK.Text = Res.Get(StringId.OKButtonText); this.buttonCancel.Text = Res.Get(StringId.CancelButton); this.groupBoxHeight.Text = Res.Get(StringId.Height); - this.labelPixels.Text = Res.Get(StringId.pixels); + this.labelPixels.Text = Res.Get(StringId.Pixels); this.radioButtonFixedHeight.Text = Res.Get(StringId.RowPropertiesFixedHeight); this.radioButtonSizeToContent.Text = Res.Get(StringId.RowPropertiesSizeToContent); this.Text = Res.Get(StringId.RowPropertiesTitle); diff --git a/src/managed/OpenLiveWriter.PostEditor/Tables/TablePropertiesForm.cs b/src/managed/OpenLiveWriter.PostEditor/Tables/TablePropertiesForm.cs index a792f42e..1de6f630 100644 --- a/src/managed/OpenLiveWriter.PostEditor/Tables/TablePropertiesForm.cs +++ b/src/managed/OpenLiveWriter.PostEditor/Tables/TablePropertiesForm.cs @@ -28,9 +28,9 @@ namespace OpenLiveWriter.PostEditor.Tables this.labelRows.Text = Res.Get(StringId.TableRowsLabel); this.labelColumns.Text = Res.Get(StringId.TableColumnsLabel); this.groupBoxAppearance.Text = Res.Get(StringId.Appearance); - this.labelSpacingPixels.Text = Res.Get(StringId.pixels); - this.labelPaddingPixels.Text = Res.Get(StringId.pixels); - this.labelBorderPixels.Text = Res.Get(StringId.pixels); + this.labelSpacingPixels.Text = Res.Get(StringId.Pixels); + this.labelPaddingPixels.Text = Res.Get(StringId.Pixels); + this.labelBorderPixels.Text = Res.Get(StringId.Pixels); this.checkBoxShowBorder.Text = Res.Get(StringId.TableShowBorderLabel); this.label3.Text = Res.Get(StringId.TableCellSpacingLabel); this.label1.Text = Res.Get(StringId.TableCellPaddingLabel); diff --git a/src/managed/OpenLiveWriter.Tests/OpenLiveWriter.Tests.csproj b/src/managed/OpenLiveWriter.Tests/OpenLiveWriter.Tests.csproj index 7005415f..aec7c6c7 100644 --- a/src/managed/OpenLiveWriter.Tests/OpenLiveWriter.Tests.csproj +++ b/src/managed/OpenLiveWriter.Tests/OpenLiveWriter.Tests.csproj @@ -87,6 +87,7 @@ + diff --git a/src/managed/OpenLiveWriter.Tests/PostEditor/Tables/PixelPercentTests.cs b/src/managed/OpenLiveWriter.Tests/PostEditor/Tables/PixelPercentTests.cs new file mode 100644 index 00000000..a7c4b02b --- /dev/null +++ b/src/managed/OpenLiveWriter.Tests/PostEditor/Tables/PixelPercentTests.cs @@ -0,0 +1,116 @@ +using System; +using System.Globalization; + +using NUnit.Framework; + +using OpenLiveWriter.PostEditor.Tables; + +namespace OpenLiveWriter.Tests.PostEditor.Tables +{ + [TestFixture] + public class PixelPercentTests + { + [Test] + public void Default_Constructor() + { + // Act + var sut = new PixelPercent(); + + // Assert + Assert.AreEqual(0, sut.Value); + Assert.AreEqual(PixelPercentUnits.Undefined, sut.Units); + } + + [Test] + [TestCase(1, PixelPercentUnits.Percentage)] + [TestCase(100, PixelPercentUnits.Percentage)] + [TestCase(2, PixelPercentUnits.Pixels)] + [TestCase(300, PixelPercentUnits.Pixels)] + public void Constructor_Valid_Values(int value, PixelPercentUnits units) + { + // Act + var sut = new PixelPercent(value, units); + + // Assert + Assert.AreEqual(value, sut.Value); + Assert.AreEqual(units, sut.Units); + } + + [Test] + [TestCase(-1, PixelPercentUnits.Percentage)] + [TestCase(101, PixelPercentUnits.Percentage)] + [TestCase(-1, PixelPercentUnits.Pixels)] + public void Constructor_Invalid_Values(int value, PixelPercentUnits units) + { + // Assert + Assert.Throws(() => new PixelPercent(value, units)); + } + + [Test] + [TestCase(null)] + [TestCase("")] + public void Constructor_EmptyValues_Gives_Undefined(string text) + { + var sut = new PixelPercent(text, CultureInfo.InvariantCulture); + + // Assert + Assert.AreEqual(PixelPercentUnits.Undefined, sut.Units); + } + + [Test] + [TestCase("1", 1, PixelPercentUnits.Pixels)] + [TestCase("1%", 1, PixelPercentUnits.Percentage)] + [TestCase(" 100% ", 100, PixelPercentUnits.Percentage)] + public void Constructor_Valid_Values(string text, int expectedValue, PixelPercentUnits expectedUnits) + { + // Act + var sut = new PixelPercent(text, CultureInfo.InvariantCulture); + + // Assert + Assert.AreEqual(expectedUnits, sut.Units); + Assert.AreEqual(expectedValue, sut.Value); + } + + [Test] + [TestCase("1", 1, PixelPercentUnits.Pixels)] + [TestCase("1", 1, PixelPercentUnits.Percentage)] + [TestCase(" 100 ", 100, PixelPercentUnits.Percentage)] + public void Constructor_Valid_Values_With_Units(string text, int expectedValue, PixelPercentUnits units) + { + // Act + var sut = new PixelPercent(text, CultureInfo.InvariantCulture, units); + + // Assert + Assert.AreEqual(expectedValue, sut.Value); + } + + [Test] + [TestCase("d", PixelPercentUnits.Pixels)] + [TestCase("1%", PixelPercentUnits.Percentage)] + [TestCase("100.454 ", PixelPercentUnits.Percentage)] + public void Constructor_Invalid_Values_With_Units(string text, PixelPercentUnits units) + { + // Act + var sut = new PixelPercent(text, CultureInfo.InvariantCulture, units); + + // Assert + Assert.AreEqual(0, sut.Value); + Assert.AreEqual(PixelPercentUnits.Undefined, sut.Units); + } + + [Test] + [TestCase("0", true)] + [TestCase("100", true)] + [TestCase("100%", true)] + [TestCase("x", false)] + [TestCase("107.9", false)] + public void CanParse_Values(string text, bool expected) + { + // Act + var result = PixelPercent.CanParse(text); + + // Assert + Assert.AreEqual(expected, result); + } + } +} \ No newline at end of file