Refactor of dump procedure into smaller components #76
Conversation
… to manage are arguments
- split EnsureDiscInformation into an additional EnsureCorrectInformationForSystemAndMediaType - proof of concept of using custom extensions to enum types to give better functionality (and encapsulation) - changed cmb_MediaType to keep a List<MediaType?> and got rid of Tuple<string, MediaType?>
|
|
||
| private DumpEnvironment _env; | ||
|
|
||
|
|
There was a problem hiding this comment.
Nit: Since we have a lot of locals now, might be worth reordering them and grouping them for clarification.
Secondary even more minor nit: generally don't have double lines between stuff. If grouping is needed, we can use the #region and #endregion tags
MainWindow.xaml.cs
Outdated
| private List<KeyValuePair<string, MediaType?>> _mediaTypes { get; set; } | ||
| private Process childProcess { get; set; } | ||
| private List<MediaType?> _mediaTypes { get; set; } | ||
| //private Process childProcess { get; set; } |
There was a problem hiding this comment.
Any particular reason why childProcess is being kept around? Or will that be removed in a future update?
|
|
||
| // Paths to tools | ||
| env.subdumpPath = _options.subdumpPath; | ||
| env.psxtPath = _options.psxtPath; |
There was a problem hiding this comment.
After my change goes in, all references to psxt can go away 👍
MainWindow.xaml.cs
Outdated
| /// <summary> | ||
| /// | ||
| /// </summary> | ||
| private Result EnsureCorrectInformationForSystemAndMediaType(KnownSystem? system, MediaType? type) |
There was a problem hiding this comment.
Maybe rename to something like GetSupportStatus? The full info can go in the summary
Tasks.cs
Outdated
|
|
||
| namespace DICUI | ||
| { | ||
| class Result |
There was a problem hiding this comment.
IIRC, default visibility of classes is internal. It's not an official thing, but I tend to like to explicitly put access modifiers on everything. In this case, I'd call it public. Not necessary, but fun.
Tasks.cs
Outdated
| public static implicit operator bool(Result result) => result.success; | ||
| } | ||
|
|
||
| class DumpEnvironment |
There was a problem hiding this comment.
Ditto on access modifier here.
Tasks.cs
Outdated
| } | ||
| } | ||
|
|
||
| class Tasks |
There was a problem hiding this comment.
Ditto on access modifier here.
Utilities/Converters.cs
Outdated
| using System; | ||
| using System.ComponentModel; | ||
| using System.Reflection; | ||
| using System.Globalization; |
There was a problem hiding this comment.
Nit: Good practice for namespace ordering is alphabetical, except that all System.* and Microsoft.* go first
There was a problem hiding this comment.
I never add an using directive by hand so I guess MSVS did it wrong.
|
|
||
| public static class KnownSystemExtensions | ||
| { | ||
| public static bool DoesSupportDriveSpeed(this KnownSystem? system) |
There was a problem hiding this comment.
Are you also going to add a Name extension here (for this PR)? Or will that be coming later?
There was a problem hiding this comment.
yes the idea was to move Name (and all related infos) but we don't need to do it at once. I was waiting to see if we all like the change.
|
Looks good! |
2 participants
This PR includes some refactors aimed to have a more modular code and be able to integrate the stdout of DIC process inside DICUI itself in the short future.
Tasksclass has been created to implement all the dump single tasksResultclass has been created to managed a generic success/failure value with optional message and convertble toboolMediaTypeandKnownSystemenums to reduce the amount of static function calls around (and keep code more object oriented)StartDumpinghas been split into many smaller chunks of code which are then invoked one after the otherEnsureDiscInformationhas been split intoEnsureCorrectInformationForSystemAndMediaTypewhich takes care of checking system and media typecmb_MediaTypecombobox model now stores aList<MediaType?>instead that aList<KeyValuePair<string, MediaType>>, the display name is now rendered through a converter which is bound to the data template of the combobox itself. This simplifies the code a lot and it's extensible to KnownSystem too in the short term if we like it