6
\$\begingroup\$

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!

\$\endgroup\$
6
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ public int VoteCount { get; private set; } = 0; I saw this recently but was not sure if this was ideal for this case. Now I know that it would be. Thanks for showing me this. public DateTime TimeDateCreated { get; } = DateTime.UtcNow; Classes and constructors are still new to me. I wasn't sure if this had to be set in the constructor. Another good point I need to learn. DateTimeCreated was the original name, but I thought to change it to what you see now thinking that might be confusing or too similar to the DateTime class. Guess I was wrong. \$\endgroup\$ – Milliorn Mar 20 at 14:26
  • \$\begingroup\$ 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.I didn't account for that. I'll have to think of a way to refactor that. Good catch and thank you for your help! \$\endgroup\$ – Milliorn Mar 20 at 14:31
  • \$\begingroup\$ You're welcome :) \$\endgroup\$ – potato Mar 20 at 14:32
  • \$\begingroup\$ if (VoteCount == 0) VoteCount++; I added this to .VoteUp() and something similar to VoteDown(). Either this, or put one method inside of these and route this out there. I thought this was the easiest way to go about it at least. \$\endgroup\$ – Milliorn Mar 20 at 14:41
3
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ How does your suggestion prevent the user from casting more than one up or down vote? \$\endgroup\$ – Milliorn Mar 21 at 1:23
  • \$\begingroup\$ @Milliorn I'm assign 1 and 0 and not incrementing. So, user can upvote, and down-vote whenever needed. if user up or dow voted more than once, it will just take the first one, and skip the rest. \$\endgroup\$ – iSR5 Mar 21 at 1:30
  • 1
    \$\begingroup\$ @Milliorn correction, I've totally missed one senario, where if the user is down-voted (so VoteCount would store one value of -1,0,1. I've updated the code as well. \$\endgroup\$ – iSR5 Mar 21 at 1:40
  • \$\begingroup\$ I went with your suggestion on using CreatedOn . It describes exactly what it is. It took me a few times of reading what you said and what the lesson said to understand why you suggested VoteUp() and VoteDown(). Although I have some disagreements with this approach I went ahead and changed it to your suggestion since it functions the same way with less code. The instructor of the lesson doesn't make anymore details on what to expect and it still complies with the demands of the lesson. \$\endgroup\$ – Milliorn Mar 21 at 20:11
  • \$\begingroup\$ if(string.IsNullOrEmpty(title)) { throw new ArgumentNullException(nameof(title)); } I did not think about this case. Good catch and suggestion! \$\endgroup\$ – Milliorn Mar 21 at 20:13
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I went ahead with the suggestion CreatedOn. I think it describes the property for what it is(or is it a field). I bookmarked those links. They will be useful later on. I changed the Post class to internal class Post. I am still learning all the access modifiers. I changed CreateOn to public DateTime CreatedOn { get; } = DateTime.UtcNow;. However, I am unsure if that makes it readonly. If not, I believe then it would be if I set DateTime.UtcNow in the constructor. \$\endgroup\$ – Milliorn Mar 21 at 20:28
  • \$\begingroup\$ As for ModifiedDate I like that suggestion and it be necessary if this was to be built on. Since the instructor did not request that I am leaving it now. Same goes with with the Voting By User (Version 2?). Both are required to make this work if it was to be taken further. However, I am leaving it out since it was not requested and the scope of this I believe was to just try to teach how to keep this in a valid state. Thank you for your continued help @Rick Davin. \$\endgroup\$ – Milliorn Mar 21 at 20:32
  • 1
    \$\begingroup\$ Yes @Milliorn, this is both read-only and auto-initialized: public DateTime CreatedOn { get; } = DateTime.UtcNow; \$\endgroup\$ – Rick Davin Mar 21 at 21:56
1
\$\begingroup\$

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;
            }
        }
    }
}
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.