Ревью кода проекта на Windows Phone 7. #1

 

Виталий Басов

Как и в большинстве компаний, мы проводим внутреннюю проверку качества кода. Без сомнения эта практика полезна как начинающим разработчикам так и уже устоявшимся профессионалам.

Ниже первый материал из новой серии статей.

Шаблон MVVM

Пример 1

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

Пример 2

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

Что не так с этими примерами? Казалось бы обычная инициализация контролов. Но если задуматься о том, что этот код находится во ViewModel, то становится видна проблема. Во-первых, ViewModel занимается не своим делом – работает с визуальными аспектами. Во-вторых, в коде жестко зашиты значения и для потребителя этого контрола теряется возможность влиять на внешний вид.
Согласно шабону MVVM, все визуальные аспекты должны определяться во View. Это продиктовано соображениями декомпозиции и слабой связанности. Для установки свойств предпочтительным способом является декларативный.

Константы

var Items = new List<Item>
             {
            new RubricItem
                {
                    Title = "Новости",
                   SectionType = SectionType.News
                },

В данном примере свойство Title инициализируется литералом по месту. В общем случае такой прием является плохой практикой. Когда такие строки предназначены для пользовательского интерфейса, последующая локализация может превратиться в головную боль. Чтобы облегчить свою долю, лучше оформлять строки в виде констант. Если говорить в общем, то использование констант улучшает читаемость и облегчает рефакторинг.

Перечисления

Для целей идентификации, вместо констант более оправданно использовать перечисления. Тем самым мы понижаем вероятность внесения ошибок. Как и в случае с константами, перечисление определяется однажды, а в качестве бонуса мы получаем уникальность ключа. Если позволяет логика приложения, то в качестве значения ключа лучше использовать целые числа, а не строки – сравнение строк более тяжелая операция.

В примере ниже, класс CacheKeys следует определить в виде перечисления и использовать целочисленный тип в качестве базового.

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

При разработке часто возникают ситуации, когда один и тот же алгоритм с небольшими вариациями определяется в нескольких местах. Такие случаи надо выявлять и приводить к одному знаменателю. В зависимости от ситуации, решать эту задачу можно по-разному:

  1. Оформить повторяющийся участок кода в виде функции, возможно с параметрами
  2. Создать виртуальную функцию с переопределением в наследнике, когда в определенной последовательности работают базовый вариант и/или вариант наследника.
  3. Шаблонный метод с использованием наследования, контракта или делегатов

Проблему ниже можно решить через обобщенную функцию вида 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;
}

Функциональная декомпозиция

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);
}

В даном примере, функция DeserializeNews занимается банальным пробросом вызова и используется только в одном месте. Такая функция выглядит неоправданным усложнением.

Выполнять декомпозицию кода на отдельные функции имеет смысл в случае:

  1. Повторного использования
  2. Разбиения излишне большой функции для повышения читаемости
  3. Выделения логически завершенного действия

Не стоит заниматься функциональной декомпозицией на таком уровне заранее. Лучше чтобы этот процесс происходил естественным образом в ходе разработки. В таком случае функции будут появляться по мере необходимости.

Компонентная декомпозиция

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();
  }
}

В данном примере видно, что часть логики работы компонента для кэширования данных, находится во ViewModel, что конечно же не правильно. Хорошо спроектированный компонент не должен выставлять детали внутренней реализации, и тем более отдавать ее вовне.

Паттерн Фабричный метод

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

Задача фабричного метода – создать экземпляр подходящего класса на основе входных данных. Здесь же вызывающий сам решает какой класс надо сконструировать, что не соответствует применяемому шаблону. Плюс зачем то уточняется тип аргумента через параметр шаблона. Либо неверно выбрано название класса либо неверно применен фабричный метод.