C# Better structure in my code & any other critiques

Question

Any advice on how to make this code; cleaner, more effective, just overall better!

Program creates a 'post' in the console. The post has a:

1. message
2. create on date
3. score for 'upvotes' and 'downvotes'

Users are able to upvote or downvote the post using upvote or downvote with a simple trim and to lower applied to the string.

Any feedback is good feedback. I am currently a working programmer but being self-taught the confidence is not always there..

Want to see it in action? https://repl.it/repls/RoughGhostwhiteApplet

namespace StackOverflowPost
{
 class Program
 {
 static void Main(string[] args)
 {
 Console.WriteLine("Write a post!");
 string post = Console.ReadLine();
 Post newPost = new Post();
 newPost.AddPostMessage(post);
 
 bool val = true;
 while (val)
 {
 newPost.ShowPost();
 Console.WriteLine("What would you like to do now? \n You can 'UpVote', 'DownVote'");
 string inputData = Console.ReadLine();
 string cleanData = inputData.Trim().ToLower();
 if (cleanData == "upvote")
 {
 newPost.Upvote();
 }
 else if (cleanData == "downvote")
 {
 newPost.DownVote();
 }
 else
 {
 val = false;
 }
 }
 }
 class Post
 {
 int voteScore;
 string postMessage;
 DateTime postDate;
 public Post()
 {
 this.postDate = DateTime.Now;
 }
 public void AddPostMessage(string post)
 {
 postMessage = post;
 }
 public void Upvote()
 {
 voteScore++;
 }
 public void DownVote()
 {
 voteScore--;
 }
 public void ShowPost()
 {
 Console.WriteLine("------------------------------------");
 Console.WriteLine($"Original Post Date {postDate}");
 Console.WriteLine($"User wrote: {postMessage}");
 Console.WriteLine($"Votes: {voteScore}");
 Console.WriteLine("------------------------------------");
 }
 
 }
 }
}

• user: Instead of while (val) you could do while (true) and use break instead of a val = false
• BCdotWEB: The current question title of your question is too generic to be helpful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?.

Answer 1

Two assumptions (because it is simplet for me with them)

1. Code works as it should
2. This code should be prepared to go "the enterprise road" (e.g. we assume many people maintaining it for a long time in the future).

Note belows are opinions only, please do not treat is as a source of truth because your company might have a different guidelines. Since it is C# code I'm trying to stick to MSFT gudielines and, where not possible, with my own preference.

Overall comments

• I'm really happy that you provided a working 'ready to run' example.
• I think you should start thinking about unit testing your code if you haven't already. Simple unit tests as a start. For example 'does Upvote() increases the score`'.
• I really like 'make your functions as short as it is convinient and logical' approach to writing code. Here, it would mean that ideally we would split the main function to a couple of small ones. I would be very happy if you refactored it to small logical functions and posted a new question so we can pick it up from there. I would suggest something similar to the below.

Main(){
 WriteWelcomeMessage();
 var userInput = ReadUserInput();
 var newPost = CreateNewPost(userInput);
 while(val){
 DisplayPost(newPost);
 userInput = ReadUserInput();
 ExecutePostAction(userInput, newPost);
 }
}

Or something similar, I think you get the idea. This can be further split so Main has only two or one function calls. I consider this approach an implementation of separation of concerns and single responsibility principle.

• This is still early in the project but if you would proceed with this as an enterprise application I would suggest reading on dependency injection. Here you can inject classes like reader, writer, postFactory or similar. This will make the testing much simpler in the future.
• I highly recommend to include some style checker (e.g. stylecop) this will ensure that you can spot more guidelines issues.

Main function

• Don't couple your implementation to Console class. I personally would write a class that would encapsulate writing/reading (or maybe two classes?) so you can easily replace it with for example reading from a WPF textbox or writing to a file.

• I strongly believe in descriptive variable naming, variable always should be describing its content. Below variables are IMHO missnamed:

  • post - should be 'postContent'
  • val - shouldContinue (?)
  • inputData - userChoice

• If you would rename the variables to be descriptive, you can drop type declarations. string or bool are not giving much context. See here for more info.

Post class

• I like that you encapsulated upvoting and downvoting to functions.
• Is there any reason why content of the post is not passed through the constructor? This might lead to null reference exception if the user of this class would forget to call AddPostMessage.
• I would add private infront of the fields
• You could inject a writer class to Post in order to decouple it from Console class.
• You could also inject some DateTime provider in order to unit test this class easier.
• I prefer using DateTime.UtcNow instead, this makes handling multiple timezones easier.

Answer 2

My take: ergonomics.

To improve user experience and keeping in mind the constraints of a console application, I would minimize the number of keystrokes required to operate the program. To downvote or upvote a post, pressing u or d respectively should be sufficient.

The actions on the posts (currently: downvote, upvote) could be made an Enum. It is possible that in the future, as your application grows you will want to add more actions eg. flag post, delete etc. I would build the app with flexibility and mind. So the Downvote action should be mapped to letter d etc.

But an Enum must contain integral values so we have to find another alternative. Here is my try inspired on this post. Note the conversion of char to string in the switch block - required for comparing values. Response is converted to lowercase too.

TODO: wrap the code in a while loop to handle invalid responses.

using System;
class MainClass {
 /* possible actions on a post */
 public static class PostAction
 {
 public const string 
 Downvote = "d",
 Upvote = "u";
 }
 public static void Main (string[] args) {
 char response;
 Console.WriteLine("What would you like to do now? \nYou can 'UpVote', 'DownVote'");
 response = Console.ReadKey().KeyChar;
 response = char.ToLower(response);
 Console.WriteLine("You answered: {0}", response);
 switch (response.ToString())
 {
 case PostAction.Downvote:
 Console.WriteLine("Downvote");
 break;
 case PostAction.Upvote:
 Console.WriteLine("Upvote");
 break;
 default:
 Console.WriteLine("Not a valid answer");
 break;
 }
 }
}