Skip to content

[Harsh Review] Add PrintNumbers.cs - #3

Open
kendaleiv wants to merge 1 commit into
mainfrom
cs-print-numbers
Open

[Harsh Review] Add PrintNumbers.cs#3
kendaleiv wants to merge 1 commit into
mainfrom
cs-print-numbers

Conversation

@kendaleiv

Copy link
Copy Markdown
Owner

No description provided.

@kendaleiv kendaleiv left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address my comments.

Add a PR description, it's blank. Amend your commit to include the same text.

Comment thread PrintNumbers.cs
@@ -0,0 +1,10 @@
public class PrintNumbers
{
public void Print()

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no tests for this method. Please add them.

Comment thread PrintNumbers.cs
@@ -0,0 +1,10 @@
public class PrintNumbers
{
public void Print()

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method isn't invoked.

Comment thread PrintNumbers.cs
@@ -0,0 +1,10 @@
public class PrintNumbers
{
public void Print()

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method could be static.

Comment thread PrintNumbers.cs
@@ -0,0 +1,10 @@
public class PrintNumbers

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class could be static.

Comment thread PrintNumbers.cs
{
public void Print()
{
for (var i = 0; i < 10; i++)

@kendaleiv kendaleiv Oct 21, 2024

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't only count to 9, it should accept the number to count to as a parameter.

Comment thread PrintNumbers.cs
{
for (var i = 0; i < 10; i++)
{
System.Console.WriteLine(i);

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move System to a using.

Comment thread PrintNumbers.cs
System.Console.WriteLine(i);
}
}
} No newline at end of file

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file doesn't end with a newline.

Comment thread PrintNumbers.cs
@@ -0,0 +1,10 @@
public class PrintNumbers

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include a namespace.

Comment thread PrintNumbers.cs
@@ -0,0 +1,10 @@
public class PrintNumbers
{
public void Print()

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no documentation for how to use this.

Comment thread PrintNumbers.cs
@@ -0,0 +1,10 @@
public class PrintNumbers

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to DisplayNumbers. Rename the method. Rename the file. Update the PR title.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant