1

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.

  • whoops sorry, just fixed that, I just wrote this up in text editor. More wondering about when to use and when not to use parameters. – Mwspencer Aug 20 '18 at 22:34
  • If you google "stateless objects vs stateful objects" you'll find a great deal of discussion on this topic. – BobRodes Aug 21 '18 at 7:34
  • @BobRodes thank you, that was exactly what I needed, I was not sure how to phase the query, and therefore was not able to find the answer with a simple google search. – Mwspencer Aug 21 '18 at 14:53
  • Understandable. The terms aren't very intuitive! – BobRodes Aug 21 '18 at 20:05
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.

  • 1
    However, one criticism of your implementation of MVVM is that there is no property named Model in your view model. -- The purist in me wants to say that, if you want to expose Model fields to the UI, then you do it using first-class properties in the ViewModel, not by exposing the entire Model for consumption by the UI. – Robert Harvey Sep 20 '18 at 16:20
  • @RobertHarvey, I went through a few of the training courses from Microsoft on this topic, and they stressed that the Model property is a defining feature of MVVM. Wrapping properties in your Model was considered a waste of code since the code already existed. That's where my perspective is coming from. I really don't hold any hard opinions on the subject. – Berin Loritsch Sep 20 '18 at 17:02
  • In practice, unless your application's UI follows a strict CRUD model, you're going to want the flexibility of, for example, exposing an Invoice View Model object that contains all of the components of an invoice, including the header, line items, total block, etc. Microsoft typically uses Entity Framework as a Model, so they would naturally gravitate towards the use of a DataContext as a Model object. Lately, I've begun to favor micro ORM's and real SQL for this purpose instead of more complex ORMs line EF and Hibernate. – Robert Harvey Sep 21 '18 at 7:26
  • I should clarify that the Invoice object is actually a property on the View Model. The UI will also need properties to populate dropdown lists and the like. – Robert Harvey Sep 21 '18 at 17:00
0

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.

  • interesting, I guess I always use my ViewModel as a gateway from the view to the model, not actually implementing much functionality, and treat the model as not only a class to hold the data, but also retrieve and manipulate the data. Any logic needed among multiple models will be in a helper class. Maybe I've been giving too much responsibility too my models or is that more a matter of opinion? – Mwspencer Aug 21 '18 at 15:53
0

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.

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.