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
ViewModelFactory.Instance.GetViewModel<NewsItem, NewsItemViewModel>(newsItem);
newsItemViewModel.Width = 310;
mainNews.Children.Add(newsItemViewModel);
Example 2
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
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.
{
/*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:
- Place a recurring part of code in a function perhaps with parameters
- Create a virtual function with overriding in inheritor when in certain sequence the basic variant and/or the inheritor variant works
- Template method using inheritance, contract or delegates
- …
The problem below can be solved using the generalized functions like IList DeserializeItems()
{
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:
- Reuse
- Separation of too large function for improving readability
- 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
{
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
{
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.
This entry was posted on Tuesday, July 24th, 2012 at 8:21 am and is filed under Code Review, Windows Phone 7.
