Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

##Naming

Naming

There are .NET Naming Guidelines which state that methods should be named using PascalCase casing. You haven't done this for buildString().

##The code

The code

  • The InnerXml property returns a string hence there is no need to call ToString() on the property.
  • Because all methods are public you should do proper parameter validation. At least you should check if the passed parameter is null and if yes throw an ArgumentNullException.
  • The PrintOutput () method does way too much. It builds the output, writes it to the console and reads a line from the console.
  • Because _root won't change you should make it readonly.
  • Calling PrintOutput twice results in doubling the printed value because you never reset/clear the StringBuilder.
  • The method name of ReturnNameAsString is too much. Because the method isn't void it is clear that it returns something. From looking at the returned type (string) it is clear what the method returns. Just call it GetNodeName instead.
  • The class name XMLRecursionReader exposes too much information about the implementation. A user of your code won't need to know that you solve the task by using recursion.
  • Both methods buildString and ReturnNameAsString should be private.
  • The if (node.HasChildNodes) in the buildString method is superflous because you are using a foreach to iterate over the ChildNodes. If there aren't any ChildNodes the foreach won't iterate.
  • The constructor assumes that the passed XmlDocument containes ChildNodes which may not be the case.

##Naming

There are .NET Naming Guidelines which state that methods should be named using PascalCase casing. You haven't done this for buildString().

##The code

  • The InnerXml property returns a string hence there is no need to call ToString() on the property.
  • Because all methods are public you should do proper parameter validation. At least you should check if the passed parameter is null and if yes throw an ArgumentNullException.
  • The PrintOutput () method does way too much. It builds the output, writes it to the console and reads a line from the console.
  • Because _root won't change you should make it readonly.
  • Calling PrintOutput twice results in doubling the printed value because you never reset/clear the StringBuilder.
  • The method name of ReturnNameAsString is too much. Because the method isn't void it is clear that it returns something. From looking at the returned type (string) it is clear what the method returns. Just call it GetNodeName instead.
  • The class name XMLRecursionReader exposes too much information about the implementation. A user of your code won't need to know that you solve the task by using recursion.
  • Both methods buildString and ReturnNameAsString should be private.
  • The if (node.HasChildNodes) in the buildString method is superflous because you are using a foreach to iterate over the ChildNodes. If there aren't any ChildNodes the foreach won't iterate.
  • The constructor assumes that the passed XmlDocument containes ChildNodes which may not be the case.

Naming

There are .NET Naming Guidelines which state that methods should be named using PascalCase casing. You haven't done this for buildString().

The code

  • The InnerXml property returns a string hence there is no need to call ToString() on the property.
  • Because all methods are public you should do proper parameter validation. At least you should check if the passed parameter is null and if yes throw an ArgumentNullException.
  • The PrintOutput () method does way too much. It builds the output, writes it to the console and reads a line from the console.
  • Because _root won't change you should make it readonly.
  • Calling PrintOutput twice results in doubling the printed value because you never reset/clear the StringBuilder.
  • The method name of ReturnNameAsString is too much. Because the method isn't void it is clear that it returns something. From looking at the returned type (string) it is clear what the method returns. Just call it GetNodeName instead.
  • The class name XMLRecursionReader exposes too much information about the implementation. A user of your code won't need to know that you solve the task by using recursion.
  • Both methods buildString and ReturnNameAsString should be private.
  • The if (node.HasChildNodes) in the buildString method is superflous because you are using a foreach to iterate over the ChildNodes. If there aren't any ChildNodes the foreach won't iterate.
  • The constructor assumes that the passed XmlDocument containes ChildNodes which may not be the case.
added 109 characters in body
Source Link
Heslacher
  • 50.9k
  • 5
  • 83
  • 177

##Naming

There are .NET Naming Guidelines which state that methods should be named using PascalCase casing. You haven't done this for buildString().

##The code

  • The InnerXml property returns a string hence there is no need to call ToString() on the property.
  • Because all methods are public you should do proper parameter validation. At least you should check if the passed parameter is null and if yes throw an ArgumentNullException.
  • The PrintOutput () method does way too much. It builds the output, writes it to the console and reads a line from the console.
  • Because _root won't change you should make it readonly.
  • Calling PrintOutput twice results in doubling the printed value because you never reset/clear the StringBuilder.
  • The method name of ReturnNameAsString is too much. Because the method isn't void it is clear that it returns something. From looking at the returned type (string) it is clear what the method returns. Just call it GetNodeName instead.
  • The class name XMLRecursionReader exposes too much information about the implementation. A user of your code won't need to know that you solve the task by using recursion.
  • Both methods buildString and ReturnNameAsString should be private.
  • The if (node.HasChildNodes) in the buildString method is superflous because you are using a foreach to iterate over the ChildNodes. If there aren't any ChildNodes the foreach won't iterate.
  • The constructor assumes that the passed XmlDocument containes ChildNodes which may not be the case.

##Naming

There are .NET Naming Guidelines which state that methods should be named using PascalCase casing. You haven't done this for buildString().

##The code

  • The InnerXml property returns a string hence there is no need to call ToString() on the property.
  • Because all methods are public you should do proper parameter validation. At least you should check if the passed parameter is null and if yes throw an ArgumentNullException.
  • The PrintOutput () method does way too much. It builds the output, writes it to the console and reads a line from the console.
  • Because _root won't change you should make it readonly.
  • Calling PrintOutput twice results in doubling the printed value because you never reset/clear the StringBuilder.
  • The method name of ReturnNameAsString is too much. Because the method isn't void it is clear that it returns something. From looking at the returned type (string) it is clear what the method returns. Just call it GetNodeName instead.
  • The class name XMLRecursionReader exposes too much information about the implementation. A user of your code won't need to know that you solve the task by using recursion.
  • Both methods buildString and ReturnNameAsString should be private.
  • The if (node.HasChildNodes) in the buildString method is superflous because you are using a foreach to iterate over the ChildNodes. If there aren't any ChildNodes the foreach won't iterate.

##Naming

There are .NET Naming Guidelines which state that methods should be named using PascalCase casing. You haven't done this for buildString().

##The code

  • The InnerXml property returns a string hence there is no need to call ToString() on the property.
  • Because all methods are public you should do proper parameter validation. At least you should check if the passed parameter is null and if yes throw an ArgumentNullException.
  • The PrintOutput () method does way too much. It builds the output, writes it to the console and reads a line from the console.
  • Because _root won't change you should make it readonly.
  • Calling PrintOutput twice results in doubling the printed value because you never reset/clear the StringBuilder.
  • The method name of ReturnNameAsString is too much. Because the method isn't void it is clear that it returns something. From looking at the returned type (string) it is clear what the method returns. Just call it GetNodeName instead.
  • The class name XMLRecursionReader exposes too much information about the implementation. A user of your code won't need to know that you solve the task by using recursion.
  • Both methods buildString and ReturnNameAsString should be private.
  • The if (node.HasChildNodes) in the buildString method is superflous because you are using a foreach to iterate over the ChildNodes. If there aren't any ChildNodes the foreach won't iterate.
  • The constructor assumes that the passed XmlDocument containes ChildNodes which may not be the case.
Source Link
Heslacher
  • 50.9k
  • 5
  • 83
  • 177

##Naming

There are .NET Naming Guidelines which state that methods should be named using PascalCase casing. You haven't done this for buildString().

##The code

  • The InnerXml property returns a string hence there is no need to call ToString() on the property.
  • Because all methods are public you should do proper parameter validation. At least you should check if the passed parameter is null and if yes throw an ArgumentNullException.
  • The PrintOutput () method does way too much. It builds the output, writes it to the console and reads a line from the console.
  • Because _root won't change you should make it readonly.
  • Calling PrintOutput twice results in doubling the printed value because you never reset/clear the StringBuilder.
  • The method name of ReturnNameAsString is too much. Because the method isn't void it is clear that it returns something. From looking at the returned type (string) it is clear what the method returns. Just call it GetNodeName instead.
  • The class name XMLRecursionReader exposes too much information about the implementation. A user of your code won't need to know that you solve the task by using recursion.
  • Both methods buildString and ReturnNameAsString should be private.
  • The if (node.HasChildNodes) in the buildString method is superflous because you are using a foreach to iterate over the ChildNodes. If there aren't any ChildNodes the foreach won't iterate.
default

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