Section 2: Classes - Design a StackOverflow Post

Question

Design a class called Post. This class models a StackOverflow post. It should have properties for title, description and the date/time it was created. We should be able to up-vote or down-vote a post. We should also be able to see the current vote value. In the main method, create a post, up-vote and down-vote it a few times and then display the the current vote value.

In this exercise, you will learn that a StackOverflow post should provide methods for up-voting and down-voting. You should not give the ability to set the Vote property from the outside, because otherwise, you may accidentally change the votes of a class to 0 or to a random number. And this is how we create bugs in our programs. The class should always protect its state and hide its implementation detail.

Educational tip: The aim of this exercise is to help you understand that classes should encapsulate data AND behaviour around that data. Many developers (even those with years of experience) tend to create classes that are purely data containers, and other classes that are purely behaviour (methods) providers. This is not object-oriented programming. This is procedural programming. Such programs are very fragile. Making a change breaks many parts of the code.

That is the information given to me for this exercise in my C# tutorial. The following is my working code. Still trying to grasp Object Oriented Programming. I believe I did set the properties so that you can get the information but not give the ability to set the Vote property from the outside by setting the property to Private. I also tried to protect the state of voting so that you could not up-vote or down-vote consecutively, yet allow it to change your vote and show the vote count along with protecting the state of the vote count.

Anyhow if anyone can see how to improve on this, point out a better way, or teach me something new I would greatly appreciate it.

using System;

namespace ExerciseTwo
{
    class Post
    {
        public string Title { get; set; }
        public string Description { get; set; }
        public DateTime TimeDateCreated { get; private set; }
        public int VoteCount { get; private set; }
        private bool HasVotedUp;
        private bool HasVotedDown;

        public Post(string title, string description)
        {
            Title = title;
            Description = description;
            TimeDateCreated = DateTime.UtcNow;
            VoteCount = 0;
        }

        public void VoteUp()
        {
            if (HasVotedUp)
            {
                throw new Exception("You have already up-voted.");
            }
            else
            {
                VoteCount++;
                HasVotedUp = true;
                HasVotedDown = false;
            }
        }

        public void VoteDown()
        {
            if (HasVotedDown)
            {
                throw new Exception("You have already down-voted.");
            }
            else
            {
                VoteCount--;
                HasVotedDown = true;
                HasVotedUp = false;
            }
        }
    }
}

Script to demo how it could work.

namespace ExerciseTwo
{
    class Program
    {
        static void Main()
        {
            Post post = new Post("Does my post work?", "Test to see if my post works.");
            System.Console.WriteLine($"Title: {post.Title}");
            System.Console.WriteLine($"Description: {post.Description}");
            System.Console.WriteLine($"Date Created: {post.TimeDateCreated}");
            System.Console.WriteLine($"Post Count: {post.VoteCount}");
            post.VoteDown();
            System.Console.WriteLine($"Post Count: {post.VoteCount}");
            post.VoteUp();
            System.Console.WriteLine($"Post Count: {post.VoteCount}");
        }
    }
}

Thank you for your help!

Answer 1

I don't know which version of C# you use, but as of C# 6 it's possible to set initial values on property definition, which has 2 advantages:

• Can eyeball the initial values quickly by looking at the property definitions.
• Doesn't require you to copy the same initial value assignment code into additional constructors you may create.

Example initial value definition:

public int VoteCount { get; private set; } = 0;

Also in the case of int with initial value 0 you don't have to explicitly set it because when an instance of the class is initialized all the ints are initialized with the default value 0 unless you specified otherwise. Just like the bools are false by default (you didn't set them in the constructor).

---

You could add another layer of protection to the creation date:

public DateTime TimeDateCreated { get; } = DateTime.UtcNow;

Without defining a setter, TimeDateCreated is set when you create an instance of Post and can never be changed again for that instance. It makes sense here because the only date you'd ever want to change on a post is the date it was edited.

---

Not a big deal for now, but it's better to develop early the habit of giving good variable names, this means being descriptive, concise and consistent with the naming of things around the code base. Most of your names are good, but TimeDateCreated is a bit counter-intuitive because once you'll get used to the name DateTime you'll expect this property to be named DateTimeCreated.

---

It's good that you protected VoteCount, but your vote function does something unwanted: if you first downvote and then upvote, you will be back to 0 votes but without the ability to upvote. I suggest you rethink this part of the code.

Answer 2

There is nothing to add more than @potato's answer. However, I just want to re-enforced the answer.

The naming convention for TimeDateCreated can be changed to CreatedOn or CreatedDate or any related naming for creation date. The keynote here is that you don't need to specify the datatype name in the properties as the property is public and I clearly can see its datatype. Then, why should I need to include it in the its name ? since I know the datatype, I need to know what value should this property store. So, here comes the good naming convention. Doesn't matter short or long names, as long as it's describing the role of the property clearly.

The other note is the VoteUp() and VoteDown(), there is no need for exceptions, just skip voting if user already voted.

// default : VoteCount == 0 (user did not up or down voted).
// When upvote, VoteCount == 1
// when downvote, VoteCount == VoteCount - 1
public void VoteUp()
{
    if(VoteCount == -1 || VoteCount == 0)
    {
        VoteCount += 1;
    }
}

public void VoteDown()
{
    if(VoteCount == 0 || VoteCount == 1)
    {
        VoteCount -= 1;
    }
}

Now, you can get rid of HasVotedUp and HasVotedDown. You only need to throw an exception if there is an actual process breaking. This means, exceptions used to throw an error if it's breaking one of your logic's core requirements. For instance, Post requires a title. So, every post must have a title. in this case we can do :

 public Post(string title, string description)
{
    if(string.IsNullOrEmpty(title)) { throw new ArgumentNullException(nameof(title)); }

    Title = title;
    Description = description;
    TimeDateCreated = DateTime.UtcNow;
    VoteCount = 0;
}

so, you're enforcing the requirement here. This class won't be initiated unless there is a title with at least one character.

While Voting, is an optional requirement, user can upvote, downvote, or nothing. User only can upvote or downvote once. If you throw an exception in this part, you'll break the whole process (which might store valid arguments). So, just skipping it with an if statement without any exceptions would be our best approach to not break the application.

You have to use your reasoning judgement on your code, try always to link it to a real world application or use case, this would give you a really good judgement on what you will do next.

Answer 3

You have a couple of good answers. Since you are an experienced developer who is new to C#, I will address some other things.

Things you do quite well

• Braces and indentation
• Naming (most of the time)

For the last item, most of your naming is good. As @iSR5 mentions, TimeDateCreated could have a better name. I have been programming since the 1980's, and I went through those years of the variable name including the data type and scope. With .NET, this is no longer needed, but more so with .NET and C# usage, it is frowned upon.

Helpful links

Naming Guidelines

C# Coding Conventions

Spots for improvement

I would like to see an access modifier on class Post. Either public or internal.

As @potato says, the creation date should be read-only. Likewise, you may want to add a ModifiedDate. This would be updated anytime the title and description are altered. Thinking ahead, there likely would be a Content to the post, and changing it would also affect ModifiedDate.

Voting By User (Version 2?)

Beyond that, your code looks decent. My remaining issue is the class design. A Post should have a 1-to-many relationship with users (voters). Just like here, you have created a post. I can vote on it, potato can vote on it, iSR5 votes, etc. I would think tracking the votes in your class should be redesigned to account for this.

Answer 4

Thank you to everyone that helped me out on this. This is what it has refactored into based on everyone's suggestions. Looks more concise and easy to read. Appreciate the advice so I can continue to learn C#.

using System;

namespace ExerciseTwo
{
    internal class Post
    {
        public string Title { get; set; }
        public string Description { get; set; }
        public DateTime CreatedOn { get; } = DateTime.UtcNow;
        public int VoteCount { get; private set; } = 0;

        public Post(string title, string description)
        {
            if (string.IsNullOrEmpty(title)) { throw new ArgumentNullException(nameof(title)); }

            Title = title;
            Description = description;
        }

        public void VoteUp()
        {
            switch (VoteCount)
            {
                case -1:
                case 0:
                    VoteCount += 1;
                    break;
            }
        }

        public void VoteDown()
        {
            switch (VoteCount)
            {
                case 0:
                case 1:
                    VoteCount -= 1;
                    break;
            }
        }
    }
}