Keep API simple

[yurish]

Геттеры здесь сами собой в порядок удобнее, чем рефлексия. Вы бы лучше рассказали, как вы вводили logUserAction(user) в другие классы. Делали один глобальный класс, через который проходили все экшены, или же в каждый экшн вставляли вызов этого метода?

[asolntsev]

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

[petejones83]

А сделать пару: (Название параметра: Значение параметра) — не лучше бы?
А если опять где-нибудь это логировать надо и опечатку? Опять же, это неплохо сочетается с вашей идеей проверки компилятором.

[asolntsev]

Но ведь так и есть?

[petejones83]

Нет, я имею в виду, чтобы, например, функция user.getPersonCode() возвращала не какое-то одно значение, а сразу пару ключ-значение.
Тогда не нужно вручную везде прописывать его название.

[asolntsev]

Не, нельзя. Person — это самостоятельный класс, он существует независимо от нашего логирования, у него свои задачи. Его методы уже используются другими классами, его нельзя так просто менять.

[petejones83]

Понял, логично. Незачем же делать отдельные функции для логов.

[burdakovd]

В первом варианте не было контроля со стороны компилятора.

Но во втором варианте мы зато имеем дублирование имен, причём корректность строковых констант компилятором не проверяется.

Раз уж мы всё равно имеем непроверяемые компилятором константы, то можно было бы сделать и так (третий способ):

Action<User> action = new Action(User, "Buy a ticket")
    .add("PersonCode")
    .add("PersonName")
    .add("ContactInformation.EmailAddress")
    .add("ContactInformation.Language");

  LogUtil.log(action);

Хотя второй способ позволяет именовать в логе поля не именами полей классов а в зависимости от текущего контекста, то есть более гибко, например

.add("BuyerName", user.getPersonName())

Хотя не всегда это нужно, особенно если лог используется только для отладки.

Можно ещё подумать над тем, чтобы передавать в логгер поля класса как в третьем способе (без дублирования), но с возможностью проверки компилятором — тут уже придётся извратиться.

[burdakovd]

Что-то типа (C#)

.add(p => p.getPersonCode())

А логгер (хз, насколько это реализуемо) вытащит из переданного замыкания имя поля.

При этом нет никаких строковых констант, и валидность клиентского кода проверяется компилятором.

[XuMiX]

Для C# реализуется очень красиво так:
protected void Add(this TObject loggable, Expression prop)
{
var propName = (MemberExpression)prop.Body).Member.Name;
var accessor = prop.Compile();
Logger.Log("{0}: {1}", propName, accessor(loggable));
}

[XuMiX]

Блин, карма слита, так что только вот так могу написать
protected void Add[TObject](this TObject loggable, Expression[Func[TObject, object]] prop)
{
var propName = (MemberExpression)prop.Body).Member.Name;
var accessor = prop.Compile();
Logger.Log("{0}: {1}", propName, accessor(loggable));
}

[asolntsev]

Ну вот, вы тоже попались на эту удочку.
Видите сложное решение и пытаетесь его как-то улучшить, но оно по-прежнему остаётся сложным.
В данном случае такая сложность просто не нужна. Дублирование имён — небольшая проблема, не стоит её решать, усложняя систему.

[Aivean]

На мой взгляд, более правильно было бы сделать так:
Определить интерфейс Logable, в котором определить метод

public Action getLogAction();

В LogUtil написать методы
public static Action getAction(Logable obj) {
return obj.getLogAction();
}

public static void logAction(Logable obj) {
log(getAction(obj);
}


Соответвенно, что именно логировать определять в наследниках интерфейса Logable.

Ещё вариант — поля и методы, которые необходимо логировать, аннотировать специальной аннотацией и метод log через reflection будет получать значения этих полей (не уверен, что этот метод хорош для данной задачи, но интересен).

[asolntsev]

О, ну вообще всё усложняете. См. предыдущий комментарий.

[Aivean]

А по-моему, данное «усложнение» разумно. В Вашем решении для каждого объекта независимо от класса приходится добавлять ещё один метод в класс LogUtil. При этом, если в вас 100500 классов, содержимое которых нужно логировать, у вас будет 100500 методов в классе LogUtil. Далее, если у Вас есть иерархия классов, как упростить логирование потомков за счёт повторного использования логирования предков? А как быть в случае, когда в качестве объекта логгирования приходит интерфейс, а нужна информация о действительной реализации инстанса?
Это преимущество предложенного мной варианта. Среди преимуществ Вашего вижу только одно — нет необходимости менять сигнатуру логируемых классов.

[asolntsev]

Вы что-то не так поняли. А не добавляю в LogUtil 100500 методов, там всего один метод log(Action).

[Aivean]

Да, действительно, я невнимательно прочитал код. В таком случае, у Вас контроллеры перегружены кодом, связанным с логированием. Система связана (coupled) сильнее, чем должна быть. Предположим, Вы меняете сигнатуру класса User и нужно, чтобы логировалась новая структура. Вам придётся искать все места, где вы логируете User'а и вносить туда изменения.

[asolntsev]

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

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