When to use Parameters and When Not to C#

Question

I am have an application using MVVM pattern. It takes a user ID and returns a table with the user's bookmarks. I am trying to decided if it is better practice to include parameters in my model's methods.

ViewModel:

namespace BookMarks
{
    public class UserProfileVM : Notifier 
    {

        #region Properties

        private int userID;
        public int UserID
        {
            get{return userID;}
            set
            {
                userID = value;
                OnPropertyChange("UserID")
            }
        }

        private DataTable userBookmarks
        public DataTable UserBookmarks
        {
            get{return userBookmarks;}
            set
            {
                userBookmarks = value;
                OnPropertyChange("UserBookmarks")
            }
        }
        #end regiond

        /// <summary>
        /// Use UserID to fill the UserBookmarks Table with a DataTable
        /// </summary>
        private void FillUserBookmarksTable()
        {
            User user = new User();
            user.UserID = CommonValues.UserID;
            UserBookmarks = user.GetUserBookmarks();
        }

        //Constructor
        public UserProfileVM()
        {
            FillUserBookmarksTable();
        }
    }

     public class Notifier : INotifyPropertyChange
     {
         protected void OnPropertyChange(string propertyName)
         {
             if(propertyChanged != null)
             {
                 PropertyChanged(this, new PropertyChangedEventArgs(propertyName));
             }
         }
     }
}

Model:

namespace BookMarks
{
    class User
    {
        #region Properties

        public int UserID {get; set;}

        #endRegion


        internal DataTable GetUserBookmarks()
        {

            DataTable dt = new DataTable();
            //create connection
            DB_Admin dbAdmin = new DB_Admin();

            using (SqlConnection conn = dbAdmin.ConntectToDB())
            {
                string query = @"SELECT q.QuestionTitle FROM OTSE_BookMarks b
                                 INNER JOIN OTSE_Questions q on b.UserID = q.UserID and b.QuestionID = q.QuestionID
                                 WHERE b.UserID = @UserID;";

                using (SqlCommand cmd = new SqlCommand(query, conn))
                {
                    cmd.Parameters.AddWithValue("@UserID", this.UserID);

                    conn.Open();
                    SqlDataAdapter da = new SqlDataAdapter(cmd);

                    da.Fill(dt);
                    conn.Close();
                    conn.Dispose();
                }
            }
            return dt; 
        }
    }
}

My question is on best practices. Is it okay to use class properties in my non private methods, or should I be passing my methods parameters such as:

internal DataTable GetUserBookmarks(int userId){}.

The way I wrote it at the time seemed to keep everything clean, as these methods should only be used with the instances' properties, and not outside material. Basically I don't want to be able to call userInstance.GetUserBookmarks( randomInt), it should only be called with the instance's UserID.

The drawback is if someone else is using this code, when they go to call the method, they may not know they need to define the instance's userID since it is not passed a parameter. This has happened to me a couple of times already, although the bug is easy enough to catch that I haven't worried about it.

My other thought is I should use Dependency Injection, and force the whoever is using the class to define a UserId upon construction. This still wouldn't solve everything because some properties down the line may need certain methods to calculate or retrieve and cannot be defined until a different method is called. One example might be UserRank, which would first need to find UserScore, which is not something that can be injected upon construction, but rather retrieved from a DB after construction.

Any feedback is appreciated.

Answer 1

The purpose of exposing Properties in MVVM is to provide endpoints you can bind to in the UI. If you don't need that information in the UI, then it does not have to be a property.

If you passed in the userId to your GetUserBookmarks() method you could make it static. I.e. it's not something that requires an object instance to query.

internal static DataTable GetUserBookmarks(int userId) { /* .... */ }

However, one criticism of your implementation of MVVM is that there is no property named Model in your view model. That's one of the defining pieces of MVVM, allowing you to bind your UI directly to your model, reserving the properties in the ViewModel strictly to support UI functionality.

---

NOTE: the Repository Pattern is also a useful way to separate your data access from the model itself.

Answer 2

I'd use a repository pattern to do the query on the view model and pass the user id to the query. Then your User model could just hold the collection of bookmarks which would be passed in to the User constructor.

Answer 3

There is nothing wrong with having a property like UserId in the view model.

If it is needed for binding data directly to UI elements, then make it public.

If it is not needed for direct data binding, then make it private.

Looking at your code, it does not look like it is needed directly, so make it private. If possible I would include it as a parameter to the constructor, and keep the UserId as an implementation detail of the view model.