Skip to main content
Code Review

Return to Answer

Bounty Awarded with 50 reputation awarded by Community Bot
Corrected spelling
Source Link
holroy
  • 11.8k
  • 1
  • 27
  • 59

Focusing on the helper methods only.

Lets start with the naming of the private static methods to get this out of our way.

MethodnamesMethod names starting with an underscore is IMO a no go. Based on the NET naming guidelines one shouldn't use any underscores in the names. One exception though is the use of a prefixing underscore for class level variables.
It just looks weird if you call call such methods.


private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
 var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);
 if (htmlAttributesDictionary.ContainsKey("disabled"))
 {
 htmlAttributesDictionary.Remove("disabled");
 }
 htmlAttributesDictionary.Add("disabled", "disabled");
 return htmlAttributesDictionary;
}

There is no need to use ContainsKey() before you use Remove() because if there is no entry with the passed key the method will just return without throwing an exception.
Because Because you add the same key later, you could simplify this by just using the setter of the Index property like so

private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
 var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);
 htmlAttributesDictionary["disabled"] = "disabled";
 return htmlAttributesDictionary;
}

If the key "disabled" isn't in the dictionary it will be added with the value "disabled" otherwise the value will be overwritten.

This tenaryternary

var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : htmlHelper.AnonymousObjectToHtmlAttributes(attributes);

is somehowsomewhat ugly and less performant. First you are using is and if this evaluates to true you do the cast. A faster way would be to use the soft casting as and use the null-coalescing operator ?? which leads to

private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
 var htmlAttributesDictionary = attributes as IDictionary<string, object> ?? HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);
 htmlAttributesDictionary["disabled"] = "disabled";
 return htmlAttributesDictionary;
}

In all of the remaining private static methods you have this

if (!disabled)
 return renderWithObject(expression, value, attributes);

which doesn't meet the style used before where you have used braces {} which is recommended. Using braces makes your code less error prone, so do it always.

WetherWhether you decide to use them or not, you should stick to the choosenchosen style. Mixing styles makes it harder to read the code.

Focusing on the helper methods only.

Lets start with the naming of the private static methods to get this out of our way.

Methodnames starting with an underscore is IMO a no go. Based on the NET naming guidelines one shouldn't use any underscores in the names. One exception though is the use of a prefixing underscore for class level variables.
It just looks weird if you call call such methods.


private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
 var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);
 if (htmlAttributesDictionary.ContainsKey("disabled"))
 {
 htmlAttributesDictionary.Remove("disabled");
 }
 htmlAttributesDictionary.Add("disabled", "disabled");
 return htmlAttributesDictionary;
}

There is no need to use ContainsKey before you use Remove() because if there is no entry with the passed key the method will just return without throwing an exception.
Because you add the same key later, you could simplify this by just using the setter of the Index property like so

private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
 var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);
 htmlAttributesDictionary["disabled"] = "disabled";
 return htmlAttributesDictionary;
}

If the key "disabled" isn't in the dictionary it will be added with the value "disabled" otherwise the value will be overwritten.

This tenary

var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : htmlHelper.AnonymousObjectToHtmlAttributes(attributes);

is somehow ugly and less performant. First you are using is and if this evaluates to true you do the cast. A faster way would be to use the soft casting as and use the null-coalescing operator ?? which leads to

private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
 var htmlAttributesDictionary = attributes as IDictionary<string, object> ?? HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);
 htmlAttributesDictionary["disabled"] = "disabled";
 return htmlAttributesDictionary;
}

In all of the remaining private static methods you have this

if (!disabled)
 return renderWithObject(expression, value, attributes);

which doesn't meet the style used before where you have used braces {} which is recommended. Using braces makes your code less error prone, so do it always.

Wether you decide to use them or not, you should stick to the choosen style. Mixing styles makes it harder to read the code.

Focusing on the helper methods only.

Lets start with the naming of the private static methods to get this out of our way.

Method names starting with an underscore is IMO a no go. Based on the NET naming guidelines one shouldn't use any underscores in the names. One exception though is the use of a prefixing underscore for class level variables.
It just looks weird if you call call such methods.


private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
 var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);
 if (htmlAttributesDictionary.ContainsKey("disabled"))
 {
 htmlAttributesDictionary.Remove("disabled");
 }
 htmlAttributesDictionary.Add("disabled", "disabled");
 return htmlAttributesDictionary;
}

There is no need to use ContainsKey() before you use Remove() because if there is no entry with the passed key the method will just return without throwing an exception. Because you add the same key later, you could simplify this by just using the setter of the Index property like so

private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
 var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);
 htmlAttributesDictionary["disabled"] = "disabled";
 return htmlAttributesDictionary;
}

If the key "disabled" isn't in the dictionary it will be added with the value "disabled" otherwise the value will be overwritten.

This ternary

var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : htmlHelper.AnonymousObjectToHtmlAttributes(attributes);

is somewhat ugly and less performant. First you are using is and if this evaluates to true you do the cast. A faster way would be to use the soft casting as and use the null-coalescing operator ?? which leads to

private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
 var htmlAttributesDictionary = attributes as IDictionary<string, object> ?? HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);
 htmlAttributesDictionary["disabled"] = "disabled";
 return htmlAttributesDictionary;
}

In all of the remaining private static methods you have this

if (!disabled)
 return renderWithObject(expression, value, attributes);

which doesn't meet the style used before where you have used braces {} which is recommended. Using braces makes your code less error prone, so do it always.

Whether you decide to use them or not, you should stick to the chosen style. Mixing styles makes it harder to read the code.

Source Link
Heslacher
  • 50.9k
  • 5
  • 83
  • 177

Focusing on the helper methods only.

Lets start with the naming of the private static methods to get this out of our way.

Methodnames starting with an underscore is IMO a no go. Based on the NET naming guidelines one shouldn't use any underscores in the names. One exception though is the use of a prefixing underscore for class level variables.
It just looks weird if you call call such methods.


private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
 var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);
 if (htmlAttributesDictionary.ContainsKey("disabled"))
 {
 htmlAttributesDictionary.Remove("disabled");
 }
 htmlAttributesDictionary.Add("disabled", "disabled");
 return htmlAttributesDictionary;
}

There is no need to use ContainsKey before you use Remove() because if there is no entry with the passed key the method will just return without throwing an exception.
Because you add the same key later, you could simplify this by just using the setter of the Index property like so

private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
 var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);
 htmlAttributesDictionary["disabled"] = "disabled";
 return htmlAttributesDictionary;
}

If the key "disabled" isn't in the dictionary it will be added with the value "disabled" otherwise the value will be overwritten.

This tenary

var htmlAttributesDictionary = attributes is IDictionary<string, object> ? (IDictionary<string, object>)attributes : htmlHelper.AnonymousObjectToHtmlAttributes(attributes);

is somehow ugly and less performant. First you are using is and if this evaluates to true you do the cast. A faster way would be to use the soft casting as and use the null-coalescing operator ?? which leads to

private static IDictionary<string, object> _GetDisabledAttributesDictionary(object attributes)
{
 var htmlAttributesDictionary = attributes as IDictionary<string, object> ?? HtmlHelper.AnonymousObjectToHtmlAttributes(attributes);
 htmlAttributesDictionary["disabled"] = "disabled";
 return htmlAttributesDictionary;
}

In all of the remaining private static methods you have this

if (!disabled)
 return renderWithObject(expression, value, attributes);

which doesn't meet the style used before where you have used braces {} which is recommended. Using braces makes your code less error prone, so do it always.

Wether you decide to use them or not, you should stick to the choosen style. Mixing styles makes it harder to read the code.

lang-cs

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