Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

Looks pretty good. Just a few little things:

  • Command-related methods CanDoXXXX and DoXXXX would be better off named CanExecuteXXXX and ExecuteXXXX or OnExecuteXXXX.
  • I would put the ViewModel properties IsEditable and IsDirty in some IEditable 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 the app.CurrentUser, maybe the VM's dependencies are more with some ICurrentUser than the whole IApplicationManager.
  • Your ViewModelBase doesn't show its INotifyPropertyChanged implementation, I'd do something like this something like this so instead of OnPropertyChanged("CoreEditorVM") you could do OnPropertyChanged(() => 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 and DoXXXX would be better off named CanExecuteXXXX and ExecuteXXXX or OnExecuteXXXX.
  • I would put the ViewModel properties IsEditable and IsDirty in some IEditable 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 the app.CurrentUser, maybe the VM's dependencies are more with some ICurrentUser than the whole IApplicationManager.
  • Your ViewModelBase doesn't show its INotifyPropertyChanged implementation, I'd do something like this so instead of OnPropertyChanged("CoreEditorVM") you could do OnPropertyChanged(() => 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 and DoXXXX would be better off named CanExecuteXXXX and ExecuteXXXX or OnExecuteXXXX.
  • I would put the ViewModel properties IsEditable and IsDirty in some IEditable 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 the app.CurrentUser, maybe the VM's dependencies are more with some ICurrentUser than the whole IApplicationManager.
  • Your ViewModelBase doesn't show its INotifyPropertyChanged implementation, I'd do something like this so instead of OnPropertyChanged("CoreEditorVM") you could do OnPropertyChanged(() => CoreEditorVM) and have a strongly-typed way of referring to your property names.
Source Link
Mathieu Guindon
  • 75.5k
  • 18
  • 194
  • 467

Looks pretty good. Just a few little things:

  • Command-related methods CanDoXXXX and DoXXXX would be better off named CanExecuteXXXX and ExecuteXXXX or OnExecuteXXXX.
  • I would put the ViewModel properties IsEditable and IsDirty in some IEditable 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 the app.CurrentUser, maybe the VM's dependencies are more with some ICurrentUser than the whole IApplicationManager.
  • Your ViewModelBase doesn't show its INotifyPropertyChanged implementation, I'd do something like this so instead of OnPropertyChanged("CoreEditorVM") you could do OnPropertyChanged(() => CoreEditorVM) and have a strongly-typed way of referring to your property names.
lang-cs

AltStyle によって変換されたページ (->オリジナル) /