replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Looks pretty good. Just a few little things:
- Command-related methods
CanDoXXXX
andDoXXXX
would be better off namedCanExecuteXXXX
andExecuteXXXX
orOnExecuteXXXX
. - I would put the ViewModel properties
IsEditable
andIsDirty
in someIEditable
interface that applicable ViewModels implement. Looks like reusable stuff. - Your VM is tightly coupled with your log provider - I'd wrap it with some
ILogger
interface which would be constructor-injected as needed. - You're not always consistent with access modifiers: sometimes you do specify the default (
private void InitialiseProduct
), other times you leave it (void VM_PropertyChanged
). I'd specify it everywhere (be explicit). IApplicationManager
is dangerous. You're passing it as a dependency which is good, but its name (xxxxManager
) suggests that it might, over time, become a shove-it-all-in-there helper class with low cohesion. You don't show much code, but if all it's used for in the VM is to get theapp.CurrentUser
, maybe the VM's dependencies are more with someICurrentUser
than the wholeIApplicationManager
.- Your
ViewModelBase
doesn't show itsINotifyPropertyChanged
implementation, I'd do something like this something like this so instead ofOnPropertyChanged("CoreEditorVM")
you could doOnPropertyChanged(() => CoreEditorVM)
and have a strongly-typed way of referring to your property names.
Looks pretty good. Just a few little things:
- Command-related methods
CanDoXXXX
andDoXXXX
would be better off namedCanExecuteXXXX
andExecuteXXXX
orOnExecuteXXXX
. - I would put the ViewModel properties
IsEditable
andIsDirty
in someIEditable
interface that applicable ViewModels implement. Looks like reusable stuff. - Your VM is tightly coupled with your log provider - I'd wrap it with some
ILogger
interface which would be constructor-injected as needed. - You're not always consistent with access modifiers: sometimes you do specify the default (
private void InitialiseProduct
), other times you leave it (void VM_PropertyChanged
). I'd specify it everywhere (be explicit). IApplicationManager
is dangerous. You're passing it as a dependency which is good, but its name (xxxxManager
) suggests that it might, over time, become a shove-it-all-in-there helper class with low cohesion. You don't show much code, but if all it's used for in the VM is to get theapp.CurrentUser
, maybe the VM's dependencies are more with someICurrentUser
than the wholeIApplicationManager
.- Your
ViewModelBase
doesn't show itsINotifyPropertyChanged
implementation, I'd do something like this so instead ofOnPropertyChanged("CoreEditorVM")
you could doOnPropertyChanged(() => CoreEditorVM)
and have a strongly-typed way of referring to your property names.
Looks pretty good. Just a few little things:
- Command-related methods
CanDoXXXX
andDoXXXX
would be better off namedCanExecuteXXXX
andExecuteXXXX
orOnExecuteXXXX
. - I would put the ViewModel properties
IsEditable
andIsDirty
in someIEditable
interface that applicable ViewModels implement. Looks like reusable stuff. - Your VM is tightly coupled with your log provider - I'd wrap it with some
ILogger
interface which would be constructor-injected as needed. - You're not always consistent with access modifiers: sometimes you do specify the default (
private void InitialiseProduct
), other times you leave it (void VM_PropertyChanged
). I'd specify it everywhere (be explicit). IApplicationManager
is dangerous. You're passing it as a dependency which is good, but its name (xxxxManager
) suggests that it might, over time, become a shove-it-all-in-there helper class with low cohesion. You don't show much code, but if all it's used for in the VM is to get theapp.CurrentUser
, maybe the VM's dependencies are more with someICurrentUser
than the wholeIApplicationManager
.- Your
ViewModelBase
doesn't show itsINotifyPropertyChanged
implementation, I'd do something like this so instead ofOnPropertyChanged("CoreEditorVM")
you could doOnPropertyChanged(() => CoreEditorVM)
and have a strongly-typed way of referring to your property names.
Looks pretty good. Just a few little things:
- Command-related methods
CanDoXXXX
andDoXXXX
would be better off namedCanExecuteXXXX
andExecuteXXXX
orOnExecuteXXXX
. - I would put the ViewModel properties
IsEditable
andIsDirty
in someIEditable
interface that applicable ViewModels implement. Looks like reusable stuff. - Your VM is tightly coupled with your log provider - I'd wrap it with some
ILogger
interface which would be constructor-injected as needed. - You're not always consistent with access modifiers: sometimes you do specify the default (
private void InitialiseProduct
), other times you leave it (void VM_PropertyChanged
). I'd specify it everywhere (be explicit). IApplicationManager
is dangerous. You're passing it as a dependency which is good, but its name (xxxxManager
) suggests that it might, over time, become a shove-it-all-in-there helper class with low cohesion. You don't show much code, but if all it's used for in the VM is to get theapp.CurrentUser
, maybe the VM's dependencies are more with someICurrentUser
than the wholeIApplicationManager
.- Your
ViewModelBase
doesn't show itsINotifyPropertyChanged
implementation, I'd do something like this so instead ofOnPropertyChanged("CoreEditorVM")
you could doOnPropertyChanged(() => CoreEditorVM)
and have a strongly-typed way of referring to your property names.
lang-cs