Skip to main content
Code Review

Return to Answer

deleted 153 characters in body
Source Link
Nikita B
  • 13.1k
  • 1
  • 26
  • 57
public MainForm(IProcessMonitor monitor)
{
 ...
 _monitor = monitor;
 _converter = new ProcessInfoConverter();
 _monitor.Updated += OnUpdated;
 LoadProcesses();
}
private readonly IComparer<ProcessInfo> _sortingComparer;
private readonly IProcessMonitor _monitor;
private readonly ProcessInfoConverter _converter;
//or SynchronizationContext.Current, whichever you prefer.
private readonly Dispatcher _dispatcher = Dispatcher.CurrentDispatcher;

private void LoadProcesses()
{
 _monitor.ForceUpdate();
}
private void bUpdate_Click(object sender, EventArgs e)
{
 LoadProcesses();
}
private void OnUpdated()
{
 var items = _monitor.Processes
 //filtering logic
 .Where(Filter)
 //sorting logic (use IComparer instead of delegates)
 .OrderBy(_sortingComparer)
 .Select(_converter.Convert)
 .Select(GereateRowGenerateRow)
 .ToArray();
 //throw the changes back to UI thread
 _dispatcher.BeginInvoke(new Action(() =>
 {
 lvProcesses.BeginUpdate();
 lvProcesses.Items.Clear();
 lvProcesses.Items.AddRange(items);
 lvProcesses.EndUpdate();
 }));
}
 private ListViewItem GereateRowGenerateRow(Dictionary<ProcessProperties, string> properties) 
 {
 return new ListViewItem (...);
 }
 private bool Filter(ProcessInfo info) {...}
public MainForm(IProcessMonitor monitor)
{
 ...
 _monitor = monitor;
 _converter = new ProcessInfoConverter();
 _monitor.Updated += OnUpdated;
 LoadProcesses();
}
private readonly IComparer<ProcessInfo> _sortingComparer;
private readonly IProcessMonitor _monitor;
private readonly ProcessInfoConverter _converter;
//or SynchronizationContext.Current, whichever you prefer.
private readonly Dispatcher _dispatcher = Dispatcher.CurrentDispatcher;

private void LoadProcesses()
{
 _monitor.ForceUpdate();
}
private void bUpdate_Click(object sender, EventArgs e)
{
 LoadProcesses();
}
private void OnUpdated()
{
 var items = _monitor.Processes
 //filtering logic
 .Where(Filter)
 //sorting logic (use IComparer instead of delegates)
 .OrderBy(_sortingComparer)
 .Select(_converter.Convert)
 .Select(GereateRow)
 .ToArray();
 //throw the changes back to UI thread
 _dispatcher.BeginInvoke(new Action(() =>
 {
 lvProcesses.BeginUpdate();
 lvProcesses.Items.Clear();
 lvProcesses.Items.AddRange(items);
 lvProcesses.EndUpdate();
 }));
}
 private ListViewItem GereateRow(Dictionary<ProcessProperties, string> properties) 
 {
 return new ListViewItem (...);
 }
 private bool Filter(ProcessInfo info) {...}
public MainForm(IProcessMonitor monitor)
{
 ...
 _monitor = monitor;
 _converter = new ProcessInfoConverter();
 _monitor.Updated += OnUpdated;
 LoadProcesses();
}
private readonly IComparer<ProcessInfo> _sortingComparer;
private readonly IProcessMonitor _monitor;
private readonly ProcessInfoConverter _converter;
private void LoadProcesses()
{
 _monitor.ForceUpdate();
}
private void bUpdate_Click(object sender, EventArgs e)
{
 LoadProcesses();
}
private void OnUpdated()
{
 var items = _monitor.Processes
 //filtering logic
 .Where(Filter)
 //sorting logic (use IComparer instead of delegates)
 .OrderBy(_sortingComparer)
 .Select(_converter.Convert)
 .Select(GenerateRow)
 .ToArray();
 //throw the changes back to UI thread
 BeginInvoke(new Action(() =>
 {
 lvProcesses.BeginUpdate();
 lvProcesses.Items.Clear();
 lvProcesses.Items.AddRange(items);
 lvProcesses.EndUpdate();
 }));
}
 private ListViewItem GenerateRow(Dictionary<ProcessProperties, string> properties) 
 {
 return new ListViewItem (...);
 }
 private bool Filter(ProcessInfo info) {...}
added 2514 characters in body
Source Link
Nikita B
  • 13.1k
  • 1
  • 26
  • 57

this is nuts. :) Just create a proper abstractions.

It also feels like you are overusing static keyword. Why is selectedProcess static? Why are timers static? Why is list of processes static? Can they be made non-static, without modifying the rest of the code? If they can, then you should do so, it will greatly increase reusability and testability of your code. If they can't - then this is clearly a design issue. You should not rely on some global state in your code. The rule of thea thumb is:

EDIT:

I still want to see your example usage

The usage is fairly straightforward. You inject the service into your form's constructor (or instantiate it there if you do not care much about IoC) and store it as a field, say _monitor. Then you just wire your UI to appropriate methods. For example, if we take the API above, you can refresh the list as follows:

public MainForm(IProcessMonitor monitor)
{
 ...
 _monitor = monitor;
 _converter = new ProcessInfoConverter();
 _monitor.Updated += OnUpdated;
 LoadProcesses();
}
private readonly IComparer<ProcessInfo> _sortingComparer;
private readonly IProcessMonitor _monitor;
private readonly ProcessInfoConverter _converter;
//or SynchronizationContext.Current, whichever you prefer.
private readonly Dispatcher _dispatcher = Dispatcher.CurrentDispatcher;
private void LoadProcesses()
{
 _monitor.ForceUpdate();
}
private void bUpdate_Click(object sender, EventArgs e)
{
 LoadProcesses();
}
private void OnUpdated()
{
 var items = _monitor.Processes
 //filtering logic
 .Where(Filter)
 //sorting logic (use IComparer instead of delegates)
 .OrderBy(_sortingComparer)
 .Select(_converter.Convert)
 .Select(GereateRow)
 .ToArray();
 //throw the changes back to UI thread
 _dispatcher.BeginInvoke(new Action(() =>
 {
 lvProcesses.BeginUpdate();
 lvProcesses.Items.Clear();
 lvProcesses.Items.AddRange(items);
 lvProcesses.EndUpdate();
 }));
}
 private ListViewItem GereateRow(Dictionary<ProcessProperties, string> properties) 
 {
 return new ListViewItem (...);
 }
 private bool Filter(ProcessInfo info) {...}

This implementation assumes, that

  • _monitor.ForceUpdate() is asynchronous call that starts a background job of fetching information about processes.
  • once information is loaded, _monitor.Processes property is set to a new value and Updated event triggers on background thread.

Note, that timers are actually gone from UI layer. It is IProcessMonitor's job now to look for changes and fire Updated event periodically.

Active process info can be refreshed in much the same way.

P.S. This was very much coded using notepad, so I hope I didn't make too many mistakes.

this is nuts. :) Just create a proper abstractions.

It also feels like you are overusing static keyword. Why is selectedProcess static? Why are timers static? Why is list of processes static? Can they be made non-static, without modifying the rest of the code? If they can, then you should do so, it will greatly increase reusability and testability of your code. If they can't - then this is clearly a design issue. You should not rely on some global state in your code. The rule of the thumb is:

this is nuts. :) Just create proper abstractions.

It also feels like you are overusing static keyword. Why is selectedProcess static? Why are timers static? Why is list of processes static? Can they be made non-static, without modifying the rest of the code? If they can, then you should do so, it will greatly increase reusability and testability of your code. If they can't - then this is clearly a design issue. You should not rely on some global state in your code. The rule of a thumb is:

EDIT:

I still want to see your example usage

The usage is fairly straightforward. You inject the service into your form's constructor (or instantiate it there if you do not care much about IoC) and store it as a field, say _monitor. Then you just wire your UI to appropriate methods. For example, if we take the API above, you can refresh the list as follows:

public MainForm(IProcessMonitor monitor)
{
 ...
 _monitor = monitor;
 _converter = new ProcessInfoConverter();
 _monitor.Updated += OnUpdated;
 LoadProcesses();
}
private readonly IComparer<ProcessInfo> _sortingComparer;
private readonly IProcessMonitor _monitor;
private readonly ProcessInfoConverter _converter;
//or SynchronizationContext.Current, whichever you prefer.
private readonly Dispatcher _dispatcher = Dispatcher.CurrentDispatcher;
private void LoadProcesses()
{
 _monitor.ForceUpdate();
}
private void bUpdate_Click(object sender, EventArgs e)
{
 LoadProcesses();
}
private void OnUpdated()
{
 var items = _monitor.Processes
 //filtering logic
 .Where(Filter)
 //sorting logic (use IComparer instead of delegates)
 .OrderBy(_sortingComparer)
 .Select(_converter.Convert)
 .Select(GereateRow)
 .ToArray();
 //throw the changes back to UI thread
 _dispatcher.BeginInvoke(new Action(() =>
 {
 lvProcesses.BeginUpdate();
 lvProcesses.Items.Clear();
 lvProcesses.Items.AddRange(items);
 lvProcesses.EndUpdate();
 }));
}
 private ListViewItem GereateRow(Dictionary<ProcessProperties, string> properties) 
 {
 return new ListViewItem (...);
 }
 private bool Filter(ProcessInfo info) {...}

This implementation assumes, that

  • _monitor.ForceUpdate() is asynchronous call that starts a background job of fetching information about processes.
  • once information is loaded, _monitor.Processes property is set to a new value and Updated event triggers on background thread.

Note, that timers are actually gone from UI layer. It is IProcessMonitor's job now to look for changes and fire Updated event periodically.

Active process info can be refreshed in much the same way.

P.S. This was very much coded using notepad, so I hope I didn't make too many mistakes.

added 151 characters in body
Source Link
Nikita B
  • 13.1k
  • 1
  • 26
  • 57

No delegates involved and the code is much easier to navigate. Not to mention, that this will allow you to extract yet another massive code block from your oversized form.

  • you can use static for simple immutable "constants"
  • you can use static for pure functions
  • in all other cases avoid using static (yes, I'm looking at you, Service Locator)

No delegates involved and the code is much easier to navigate.

  • you can use static for simple immutable "constants"
  • you can use static for pure functions
  • in all other cases avoid using static

No delegates involved and the code is much easier to navigate. Not to mention, that this will allow you to extract yet another massive code block from your oversized form.

  • you can use static for simple immutable "constants"
  • you can use static for pure functions
  • in all other cases avoid using static (yes, I'm looking at you, Service Locator)
added 7 characters in body
Source Link
Nikita B
  • 13.1k
  • 1
  • 26
  • 57
Loading
added 11 characters in body
Source Link
Nikita B
  • 13.1k
  • 1
  • 26
  • 57
Loading
Source Link
Nikita B
  • 13.1k
  • 1
  • 26
  • 57
Loading
lang-cs

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