Code: RocketLanding
Date: 2019.01.15
Reviewer: Tiziana Brinzas

Part I
Algorithms

1. IDataSpecificInfo has a very odd interface. It should not be its concern to manage a number of devices, but simply to return one specific info.
Please change its interface to:
	LandingDeviceSpecificInformation GetDeviceSpecificInformation();

2. DataSpecificProvider should be static (only has one static method)

3. LandingDevice should not keep the Planet and BurningRate as members, these are actually characteristics of commands. Instead, the BreakCommand and FreeFall methods should take a Planet as parameter. Get rid of all the small Adjust... members, they do not bring benefit, but force you to remember the context (hence the above Planet and BurningRate at the level of LandingDevice)

4. Properties are not meant to be used exclusively with dumb get/setters. Turn GetTotalMass() and ItLandedSafely() (should be called HasLandedSafely) into get-only properties. This will come in handy later on with binding to the view model.

5. ItLandedSafely only checks the speed, but does not check if it actually landed...

6. The Land methods started to multiply... you should only have: LandCommand(Planet planet, Command command). IMHO the logic of sequencing commands belongs to the ControlTower, but if you must have LandCommand(Planet planet, IEnumerable<Command> commands) here, then make sure the single command version calls the enumeration version with a single-element array:

	public override void Land(Planet planet, Command command)
	{
		Land(planet, new[] { command });
	}

7. You have introduced a FreeFall method in LandingDevice, but in ControlTower you create a special command for that. Either have a
	public void FreeFall(Planet planet, double seconds) and use that in ControlTower as well, or get rid of it all together and just use BreakCommand.

8. Did you read about linq? If not, please go back to that. ControlTower.InitializeRockets and ControlTower.FindRocketByRocketId should be rewritten using linq. And, btw, get rid of the LandDeviceStopWatches class - you do not need it.

9. Why is the control tower assigning Ids to landing devices? Shouldn't this be done by the data source? If you need this information, then provide it (even if randomly generated) from the data source, and make sure you validate it; otherwise identify devices by reference, not by their Id.



Part II
LandingDemo

1. RelayCommand:

1.0 See https://onewindowsdev.com/2016/06/16/the-command-pattern-and-mvvm/ article for an example of using commands and their relation w. MVVM

1.1 Your implementation of ICommand is crippled: you cannot pass a CanExecute method, and you cannot pass arguments to either Execute or CanExecute.

1.2 The CanExecuteChanged event handler is supposed to notify the UI when CanExecute changes its mind, something that your implementation currently
does not allow. But commands are intended to be directly bound to, e.g. buttons; when  you do that and the command CanExecuteChanged event is raised, the button's enabled state changes accordingly. Provided, of course, that you stick to the interface (and its intentions). This means you should replace the mere declaration of the event with:

	public event EventHandler CanExecuteChanged
    {
        add { CommandManager.RequerySuggested += value; }
        remove { CommandManager.RequerySuggested -= value; }
    }

and actually implement and use the CanExecute part.

1.3 Usually a view model only needs to instantiate a command once in its lifetime. Instead of creating a new command every time your getter is called,
create it once and cache it in a private variable (see below example from our project):

	private ICommand _nextPageCommand;
	public ICommand NextPageCommand
	{
		get
		{
			return this._nextPageCommand ?? (this._nextPageCommand = new DelegateCommand(
				() => this.CurrentFrame++,
				() => this.IsImageLoaded && this.CurrentFrame < this.FrameCount - 1)
				.ObservesProperty(() => this.IsImageLoaded)
				.ObservesProperty(() => this.CurrentFrame));
		}
	}


2. ViewModelBase

2.0 See https://github.com/PrismLibrary/Prism/blob/master/Source/Prism/Mvvm/BindableBase.cs for a nice implementation of this. Please study it.

2.1 It would be better to include a more complete implementation of the basic view model, one that provides get/set implementations for properties.
It is important that you understand how notification changes are used by the binding mechanism, and that each notification is the potential source of
a cascade of notifications that eventually will refresh your UI. Consequently, all setters should check if the value actually changed before issuing a
property changed notification. And this is better done in the base class, to avoid duplicating this code all over again.

2.2 You are better off using the [CallerMemberName] attribute instead of relying on the caller to pass the correct property name to you.

2.3 Wherever you call OnPropertyChanged("<some_property>"), replace it with OnPropertyChanged(nameof(<some_property>)).
If you decide one day to rename the property, the latter version would automatically pick up the change.


3. ViewModels

3.0 I have the impression you are unclear about the roles each plays. Please see https://www.wintellect.com/model-view-viewmodel-mvvm-explained/

3.1 There seems to be no relation between your models and the corresponding view models. Normally a view model is built on top of (a) model(s), i.e.
the constructor should have some model-related argument.

3.2 I do not understand why are there a LandingDeviceViewModel and a RocketViewModel, and what is the intended relationship between them. But the RocketViewModel seems outright wrong: accessing protected/private properties defeats the purpose of encapsulation. While this is arguably acceptable in unit-tests, it is definitely not a good idea in productive code. Besides, you're even exposing properties that should not be there in the first place (see Part I, point 3)

3.3 The pattern you see illustrated via INotifyPropertyChanged is not limited, in its usage, to view models. It is actually a general use observer,
and you can make use of it at any level, for instance in the communication between the view model and model. If your model notifies of changes in its properties, and the view model listens to its properties and updates its own, you can get rid of the limited and error-prone refresh methods, which prevents you from getting "live" updates, and instead delivers fresh data only when explicitly called.

Rewrite the code to propagate changes via binding instead of explicitly calling some method, so that we get the update when the property actually changes, not when we ask of it. Both ControlTower.RefreshStatus and ControlTower.CalculateAllSafeStates methods should go away. In fact, what business does the ControlTower have calculating things that pertain exclusively to the LandingDevice??

3.4 Linq again: all calls like x.Count() > 0 should be replaced with x.Any()

3.5 Type specifiers in calls like landingDevices.ElementAt<LandingDeviceViewModel>(i) is redundant. Get rid of it.

3.6 Parentheses in constructor followed by property initializers like var rockeVM = new LandingDeviceViewModel() {...} are redundant. Get rid of it.

3.7 We discussed before about naming standards: property names should begin with capital letter, arguments should not. You are mixing it everywhere, please correct.

3.8 We discussed before about exceptions, yet you have again empty catch clauses, which will hide errors. Please correct.

3.9 Why is there a static instance of a ControlTower? You're not using it anywhere...

3.10 InitializeTower and Destroy tower have no place existing, at least not in the ControlTowerViewModel. Initializing the data source is typically done at application start, based on some external configuration. And that should cover the ControlTower instance. The view model should be able to construct itself from that.