Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

###Naming

Naming

HemEnum, or any enum name that ends with the word Enum, is a bad name for an enum. Enum types should not contain the word "enum" in their names.

Similarly, HemEnum values should not contain "Hem" in their names either. Your HemEnum should therefore read something like this:

public enum HemType
{
 None = -1,
 Sewn = 0,
 Weld = 1,
 DoubleFold = 2
}

myObject is a bad name for a class - should be MyObject. Kidding (although not really - types should be named following a PascalCasing convention). Anything that ends with "Object" should be banned from being a class name. Note that myObject doesn't compile as provided. (where's this.HemType?)

The pseudo-Hungarian notation is ok in for naming controls (i.e. the "cbo" prefix for ComboBoxes, "txt" for TextBoxes, etc.) - had you been using current technology (WPF) I would have strongly advised against such naming though.

###Nitpicks

Nitpicks

You seem to make static anything that can be made static. Don't. Just because a method doesn't use instance members now doesn't mean it never will, and changing a public static void method to be public void, is a breaking change.

vHemEnum doesn't seem to be a type defined anywhere. Typo?

I'll assume the out-of-whack indentation is a Copy+Paste glitch.

###Binding?

Binding?

You say you want databinding, and yet you're doing this:

foreach (var value in values)
{
 var s = GetHemTypeDescription((HemEnum)value.ID );
 cbo.Items.Add(s);
}

Create a type that exposes DisplayValue and EnumValue properties, and use @DanLyons' or @JesseCSlicer's answer to bind your combobox items.

###Captions

Captions

A more extensible way (vs. attributes) of providing captions for your enums, would be to use a resource file (.resx) - call the strings per the enum's names, and then retrieve the caption from the resource strings:

var description = resx.ResourceManager.GetString(hemType.ToString());

Bottom line, I've seen worse WinForms code (and WPF for that matter), but it could be improved.

###Naming

HemEnum, or any enum name that ends with the word Enum, is a bad name for an enum. Enum types should not contain the word "enum" in their names.

Similarly, HemEnum values should not contain "Hem" in their names either. Your HemEnum should therefore read something like this:

public enum HemType
{
 None = -1,
 Sewn = 0,
 Weld = 1,
 DoubleFold = 2
}

myObject is a bad name for a class - should be MyObject. Kidding (although not really - types should be named following a PascalCasing convention). Anything that ends with "Object" should be banned from being a class name. Note that myObject doesn't compile as provided. (where's this.HemType?)

The pseudo-Hungarian notation is ok in for naming controls (i.e. the "cbo" prefix for ComboBoxes, "txt" for TextBoxes, etc.) - had you been using current technology (WPF) I would have strongly advised against such naming though.

###Nitpicks

You seem to make static anything that can be made static. Don't. Just because a method doesn't use instance members now doesn't mean it never will, and changing a public static void method to be public void, is a breaking change.

vHemEnum doesn't seem to be a type defined anywhere. Typo?

I'll assume the out-of-whack indentation is a Copy+Paste glitch.

###Binding?

You say you want databinding, and yet you're doing this:

foreach (var value in values)
{
 var s = GetHemTypeDescription((HemEnum)value.ID );
 cbo.Items.Add(s);
}

Create a type that exposes DisplayValue and EnumValue properties, and use @DanLyons' or @JesseCSlicer's answer to bind your combobox items.

###Captions

A more extensible way (vs. attributes) of providing captions for your enums, would be to use a resource file (.resx) - call the strings per the enum's names, and then retrieve the caption from the resource strings:

var description = resx.ResourceManager.GetString(hemType.ToString());

Bottom line, I've seen worse WinForms code (and WPF for that matter), but it could be improved.

Naming

HemEnum, or any enum name that ends with the word Enum, is a bad name for an enum. Enum types should not contain the word "enum" in their names.

Similarly, HemEnum values should not contain "Hem" in their names either. Your HemEnum should therefore read something like this:

public enum HemType
{
 None = -1,
 Sewn = 0,
 Weld = 1,
 DoubleFold = 2
}

myObject is a bad name for a class - should be MyObject. Kidding (although not really - types should be named following a PascalCasing convention). Anything that ends with "Object" should be banned from being a class name. Note that myObject doesn't compile as provided. (where's this.HemType?)

The pseudo-Hungarian notation is ok in for naming controls (i.e. the "cbo" prefix for ComboBoxes, "txt" for TextBoxes, etc.) - had you been using current technology (WPF) I would have strongly advised against such naming though.

Nitpicks

You seem to make static anything that can be made static. Don't. Just because a method doesn't use instance members now doesn't mean it never will, and changing a public static void method to be public void, is a breaking change.

vHemEnum doesn't seem to be a type defined anywhere. Typo?

I'll assume the out-of-whack indentation is a Copy+Paste glitch.

Binding?

You say you want databinding, and yet you're doing this:

foreach (var value in values)
{
 var s = GetHemTypeDescription((HemEnum)value.ID );
 cbo.Items.Add(s);
}

Create a type that exposes DisplayValue and EnumValue properties, and use @DanLyons' or @JesseCSlicer's answer to bind your combobox items.

Captions

A more extensible way (vs. attributes) of providing captions for your enums, would be to use a resource file (.resx) - call the strings per the enum's names, and then retrieve the caption from the resource strings:

var description = resx.ResourceManager.GetString(hemType.ToString());

Bottom line, I've seen worse WinForms code (and WPF for that matter), but it could be improved.

edited per Jesse's answer
Source Link
Mathieu Guindon
  • 75.5k
  • 18
  • 194
  • 467

###Naming

HemEnum, or any enum name that ends with the word Enum, is a bad name for an enum. Enum types should not contain the word "enum" in their names.

Similarly, HemEnum values should not contain "Hem" in their names either. Your HemEnum should therefore read something like this:

public enum HemType
{
 None = -1,
 Sewn = 0,
 Weld = 1,
 DoubleFold = 2
}

myObject is a bad name for a class - should be MyObject. Kidding (although not really - types should be named following a PascalCasing convention). Anything that ends with "Object" should be banned from being a class name. Note that myObject doesn't compile as provided. (where's this.HemType?)

The pseudo-Hungarian notation is ok in for naming controls (i.e. the "cbo" prefix for ComboBoxes, "txt" for TextBoxes, etc.) - had you been using current technology (WPF) I would have strongly advised against such naming though.

###Nitpicks

You seem to make static anything that can be made static. Don't. Just because a method doesn't use instance members now doesn't mean it never will, and changing a public static void method to be public void, is a breaking change.

vHemEnum doesn't seem to be a type defined anywhere. Typo?

I'll assume the out-of-whack indentation is a Copy+Paste glitch.

###Binding?

You say you want databinding, and yet you're doing this:

foreach (var value in values)
{
 var s = GetHemTypeDescription((HemEnum)value.ID );
 cbo.Items.Add(s);
}

Create a type that exposes DisplayValue and EnumValue properties, and use @DanLyons' or @JesseCSlicer's answer to bind your combobox items.

###Captions

A more extensible way (vs. attributes) of providing captions for your enums, would be to use a resource file (.resx) - call the strings per the enum's names, and then retrieve the caption from the resource strings:

var description = resx.ResourceManager.GetString(hemType.ToString());

Bottom line, I've seen worse WinForms code (and WPF for that matter), but it could be improved.

###Naming

HemEnum, or any enum name that ends with the word Enum, is a bad name for an enum. Enum types should not contain the word "enum" in their names.

Similarly, HemEnum values should not contain "Hem" in their names either. Your HemEnum should therefore read something like this:

public enum HemType
{
 None = -1,
 Sewn = 0,
 Weld = 1,
 DoubleFold = 2
}

myObject is a bad name for a class - should be MyObject. Kidding (although not really - types should be named following a PascalCasing convention). Anything that ends with "Object" should be banned from being a class name. Note that myObject doesn't compile as provided. (where's this.HemType?)

The pseudo-Hungarian notation is ok in for naming controls (i.e. the "cbo" prefix for ComboBoxes, "txt" for TextBoxes, etc.) - had you been using current technology (WPF) I would have strongly advised against such naming though.

###Nitpicks

You seem to make static anything that can be made static. Don't. Just because a method doesn't use instance members now doesn't mean it never will, and changing a public static void method to be public void, is a breaking change.

vHemEnum doesn't seem to be a type defined anywhere. Typo?

I'll assume the out-of-whack indentation is a Copy+Paste glitch.

###Binding?

You say you want databinding, and yet you're doing this:

foreach (var value in values)
{
 var s = GetHemTypeDescription((HemEnum)value.ID );
 cbo.Items.Add(s);
}

Create a type that exposes DisplayValue and EnumValue properties, and use @DanLyons' answer to bind your combobox items.

###Captions

A more extensible way of providing captions for your enums, would be to use a resource file (.resx) - call the strings per the enum's names, and then retrieve the caption from the resource strings:

var description = resx.ResourceManager.GetString(hemType.ToString());

Bottom line, I've seen worse WinForms code (and WPF for that matter), but it could be improved.

###Naming

HemEnum, or any enum name that ends with the word Enum, is a bad name for an enum. Enum types should not contain the word "enum" in their names.

Similarly, HemEnum values should not contain "Hem" in their names either. Your HemEnum should therefore read something like this:

public enum HemType
{
 None = -1,
 Sewn = 0,
 Weld = 1,
 DoubleFold = 2
}

myObject is a bad name for a class - should be MyObject. Kidding (although not really - types should be named following a PascalCasing convention). Anything that ends with "Object" should be banned from being a class name. Note that myObject doesn't compile as provided. (where's this.HemType?)

The pseudo-Hungarian notation is ok in for naming controls (i.e. the "cbo" prefix for ComboBoxes, "txt" for TextBoxes, etc.) - had you been using current technology (WPF) I would have strongly advised against such naming though.

###Nitpicks

You seem to make static anything that can be made static. Don't. Just because a method doesn't use instance members now doesn't mean it never will, and changing a public static void method to be public void, is a breaking change.

vHemEnum doesn't seem to be a type defined anywhere. Typo?

I'll assume the out-of-whack indentation is a Copy+Paste glitch.

###Binding?

You say you want databinding, and yet you're doing this:

foreach (var value in values)
{
 var s = GetHemTypeDescription((HemEnum)value.ID );
 cbo.Items.Add(s);
}

Create a type that exposes DisplayValue and EnumValue properties, and use @DanLyons' or @JesseCSlicer's answer to bind your combobox items.

###Captions

A more extensible way (vs. attributes) of providing captions for your enums, would be to use a resource file (.resx) - call the strings per the enum's names, and then retrieve the caption from the resource strings:

var description = resx.ResourceManager.GetString(hemType.ToString());

Bottom line, I've seen worse WinForms code (and WPF for that matter), but it could be improved.

Source Link
Mathieu Guindon
  • 75.5k
  • 18
  • 194
  • 467

###Naming

HemEnum, or any enum name that ends with the word Enum, is a bad name for an enum. Enum types should not contain the word "enum" in their names.

Similarly, HemEnum values should not contain "Hem" in their names either. Your HemEnum should therefore read something like this:

public enum HemType
{
 None = -1,
 Sewn = 0,
 Weld = 1,
 DoubleFold = 2
}

myObject is a bad name for a class - should be MyObject. Kidding (although not really - types should be named following a PascalCasing convention). Anything that ends with "Object" should be banned from being a class name. Note that myObject doesn't compile as provided. (where's this.HemType?)

The pseudo-Hungarian notation is ok in for naming controls (i.e. the "cbo" prefix for ComboBoxes, "txt" for TextBoxes, etc.) - had you been using current technology (WPF) I would have strongly advised against such naming though.

###Nitpicks

You seem to make static anything that can be made static. Don't. Just because a method doesn't use instance members now doesn't mean it never will, and changing a public static void method to be public void, is a breaking change.

vHemEnum doesn't seem to be a type defined anywhere. Typo?

I'll assume the out-of-whack indentation is a Copy+Paste glitch.

###Binding?

You say you want databinding, and yet you're doing this:

foreach (var value in values)
{
 var s = GetHemTypeDescription((HemEnum)value.ID );
 cbo.Items.Add(s);
}

Create a type that exposes DisplayValue and EnumValue properties, and use @DanLyons' answer to bind your combobox items.

###Captions

A more extensible way of providing captions for your enums, would be to use a resource file (.resx) - call the strings per the enum's names, and then retrieve the caption from the resource strings:

var description = resx.ResourceManager.GetString(hemType.ToString());

Bottom line, I've seen worse WinForms code (and WPF for that matter), but it could be improved.

lang-cs

AltStyle によって変換されたページ (->オリジナル) /