"We are committed to providing our Customers with excellent software solutions.
We can handle everything - complex business, mobile, game, web applications and present perfect result at the end," Dmitry Starostenkov, CEO
 
click to start a new project
start live dialog with Enterra
 
News
About Us
Portfolio
Clients
Contact
Tech Zone

Code review for project on Windows Phone 7. #1

Vitaly Bassov

As in most companies we carry out the internal code quality checking. Without a doubt, this practice is useful as for junior developers and already well-established professionals.

Here is the first material from the new series of articles.

MVVM Template

Example 1

NewsItemViewModel newsItemViewModel =
                 ViewModelFactory.Instance.GetViewModel<NewsItem, NewsItemViewModel>(newsItem);
newsItemViewModel.Width = 310;
mainNews.Children.Add(newsItemViewModel);

Example 2

var link = new Hyperlink {
   MouseOverForeground = new SolidColorBrush(Color.FromArgb(155, 0, 102, 204)),
                                             Foreground = new SolidColorBrush(Color.FromArgb(255, 0, 102, 204))                
};

What is wrong with these examples? It would seem like normal initialization of controls. But think about the fact that this code is in the ViewModel, and you can see the problem. First, ViewModel is not dealing with its own business – working with the visual aspects. Secondly the values are hard-coded and the consumer of this control loses the opportunity to influence on the appearance.
According to MVVM template all the visual aspects should be defined in View. This is based on ideas of decomposition and weak relatedness. The preferred method for setting up properties is declarative.

Сonstants

var Items = new List<Item>
             {
            new RubricItem
                {
                    Title = "News",
                   SectionType = SectionType.News
                },

In this example the Title property is initialized as string. In general, this technique is a bad practice. When these strings are used for the user interface the subsequent localization can become a real headache. To make it easier for you it’s better to define strings as constants. Generally speaking, the use of constants improves readability and simplifies refactoring.

Enumerations

For identification purposes instead of constants it’s more reasonable to use enumerations. Thus we reduce the possibility of errors. As in case with constants enumeration is defined once, and as a bonus we get uniqueness of key. If application logics allows then it’s better to use integers for key value than strings as comparison of strings is a more difficult operation.

In the example below, CacheKeys class should be defined as enumeration and integer type should be used as basic.

public static class CacheKeys
{
    /*Items*/
    public const string NewsItemsKey = "NewsItems";
    public const string RubricItemsKey = "RubricItems";
    public const string CommentItemsKey = "CommentItems";
    public const string OfftopicItemsKey = "OfftopicItems";
    public const string GalleryItemsKey = "GalleryItems";
    public const string Favorites = "Favorites";

    /*TopicItems*/
    public const string GalleryKey = "GalleryItems";
    public const string OfftopicKey = "OfftopicItems";
    public const string CommentKey = "CommentItems";
    public const string NewsKey = "News";

    /*Main*/
    public const string MainTopicsKey = "MainTopics";
    public const string RubricsKey = "Rubrics";

    /*Rubric*/
    public const string RubricTopicsKey = "RubricTopics";
}

Copy & Paste

During the development process there are often situations when the same algorithm with minor variations is defined in multiple places. Such cases should be identified and put to a common denominator. Depending on the situation this problem can be solved in different ways:

  1. Place a recurring part of code in a function perhaps with parameters
  2. Create a virtual function with overriding in inheritor when in certain sequence the basic variant and/or the inheritor variant works
  3. Template method using inheritance, contract or delegates

The problem below can be solved using the generalized functions like IList DeserializeItems()

public IList<CommentItem> DeserializeCommentItems()
{
List<CommentItem> items = null;
var targetFileName = String.Format("{0}/{1}.dat", FolderName, CacheKeys.CommentItemsKey);

if (IsoFile.FileExists(targetFileName))
{
    try
    {
        using (IsolatedStorageFileStream sourceStream = IsoFile.OpenFile(targetFileName, FileMode.Open))
        {
            items = (List<CommentItem>) CommentItemSerializer.ReadObject(sourceStream);
        }
    }
    catch (Exception)
    {
        //TODO: log
    }
}

return items;
}


public IList<NewsItem> DeserializeNewsItems()
{
List<NewsItem> items = null;
var targetFileName = String.Format("{0}/{1}.dat", FolderName, CacheKeys.NewsItemsKey);

if (IsoFile.FileExists(targetFileName))
{
    try
    {
        using (IsolatedStorageFileStream sourceStream = IsoFile.OpenFile(targetFileName, FileMode.Open))
        {
            items = (List<NewsItem>)NewsItemSerializer.ReadObject(sourceStream);
        }
    }
    catch (Exception)
    {
        //TODO: log
    }
}

return items;
}

Functional decomposition

News news = News.Any(n => n.Id == id) ? News.FirstOrDefault(n => n.Id == id) : DeserializeNews(id);

...

private News DeserializeNews(string id)
{
return IsolatedStorageDataService.Instance.DeserializeNews(id);
}

In this example DeserializeNews function is engaged in call forwarding and is used only in one place. Such function seems like unreasonable complication.

It is reasonable to make code decomposition into separate functions in case of:

  1. Reuse
  2. Separation of too large function for improving readability
  3. Defining the logically complete action

There is no need in functional decomposition at such level in advance. It is better that this process occurs naturally through the development. In this case the functions will appear as needed.

Component decomposition

public override void LoadData()
{
  OnDataLoading();
  NewsSectionManager.Instance.LoadNews(result =>
  {
    if (result == null)
    {
    ShowNoConnectionText();
    CacheManager.Instance.SetState(CacheKeys.NewsItemsKey, true);
    return;
    }

  Items.Clear();
  foreach (NewsItem item in result)
  {
     Items.Add(ViewModelFactory.Instance.GetViewModel<NewsItem, NewsItemViewModel>(item));
     OnDataLoaded();
     if (CacheManager.Instance.NeedInvalidation(CacheKeys.NewsItemsKey))
       {
           ShowNoConnectionText();
       }
  });

  if (NewsSectionManager.Instance.CacheLoaded)
  {
    InvalidateCommand.Execute();
  }
}

In this example it is clear that part of work logics for caching data component is placed in ViewModel which is not correct for sure. A well-designed component shouldn’t expose the internal implementation details and to give it outside.

Factory Method Pattern

foreach (NewsItem item in result)
{
 Items.Add(ViewModelFactory.Instance.GetViewModel<NewsItem, NewsItemViewModel>(item));
}

The factory method aim is to create an instance of the appropriate class based on the input data. Here the caller can decide itself what class should be constructed that doesn’t comply with the used template. Also the argument type is specified via the template parameter. Either the wrong class name is selected or the factory method is used incorrectly.

© Copyright 2001-2017 Enterra Inc - Software outsourcing and Offshore Software development company.
All rights reserved. All content is copyrighted.

All trademarks mentioned in this blog belong to their respective owners.
Entries (RSS) and Comments (RSS).