Skip to main content
minor wording improvement
Source Link
ChrisWue
  • 20.6k
  • 4
  • 43
  • 107

Not sure if still relevant, but here you go:

  1. Consider refactoring your if (this.disposed) { } into a method like ThrowIfDisposed(). If you ever want to change yourthe dispose method or add logging or whatnot then you have to change only one place. Code duplication should be avoided even for trivial things like that.

  2. You swallow exceptions in Hook and Unhook and return a bool which tells you nothing except that it went wrong. Same in OpenConnection and CloseConnetion. You throw away aA lot of information is thrown away which will be useful for troubleshooting should the need arise. In general I would consider that a bad idea.

  3. Replace yourthe magic constant Thread.Sleep(100); either with a const definition or even a setting you can tweak. If you make it a setting please make it a TimeStamp - I personally find all that code which scatters around ints with implicit units very annoying. I often have to look up what it means because it's not always that obvious - seconds, milliseconds, ..?

Not sure if still relevant, but here you go:

  1. Consider refactoring your if (this.disposed) { } into a method like ThrowIfDisposed(). If you ever want to change your dispose method or add logging or whatnot then you have to change only one place. Code duplication should be avoided even for trivial things like that.

  2. You swallow exceptions in Hook and Unhook and return a bool which tells you nothing except that it went wrong. Same in OpenConnection and CloseConnetion. You throw away a lot of information which will be useful for troubleshooting should the need arise. In general I would consider that a bad idea.

  3. Replace your magic constant Thread.Sleep(100); either with a const definition or even a setting you can tweak. If you make it a setting please make it a TimeStamp - I personally find all that code which scatters around ints with implicit units very annoying. I often have to look up what it means because it's not always that obvious - seconds, milliseconds, ..?

Not sure if still relevant, but here you go:

  1. Consider refactoring if (this.disposed) { } into a method like ThrowIfDisposed(). If you ever want to change the dispose method or add logging or whatnot then you have to change only one place. Code duplication should be avoided even for trivial things like that.

  2. You swallow exceptions in Hook and Unhook and return a bool which tells you nothing except that it went wrong. Same in OpenConnection and CloseConnetion. A lot of information is thrown away which will be useful for troubleshooting should the need arise. In general I would consider that a bad idea.

  3. Replace the magic constant Thread.Sleep(100); either with a const definition or even a setting you can tweak. If you make it a setting please make it a TimeStamp - I personally find all that code which scatters around ints with implicit units very annoying. I often have to look up what it means because it's not always that obvious - seconds, milliseconds, ..?

fixed formatting and typos
Source Link
ChrisWue
  • 20.6k
  • 4
  • 43
  • 107

Not sure if still relevant, but here you go:

  1. Consider refactoring your if (this.disposed) { } into a method like ThrowIfDisposed(). If you ever want to change your dispose method or add logging or whatnot then you have to change only one place. Code duplication should be avoided eventeven for trivial things like that.

  2. You swallow exceptions in Hook and Unhook and return a bool which tells you nothing except that it went wrong. Same in OpenConnection and CloseConnetion. You throw away a lot of information which will be useful for troubleshooting should the need arise. In general I would consider that a bad idea.

  3. Replace your magic constant Thread.Sleep(100); either with a const definition or even a setting you can tweak. If you make it a setting please make it a TimeStamp - I personally find all that code which scatters around ints with implicit units very annoying. I often have to look up what it means because it's not always that obvious - seconds, milliseconds, ..?

Not sure if still relevant, but here you go:

  1. Consider refactoring your if (this.disposed) { } into a method like ThrowIfDisposed(). If you ever want to change your dispose method or add logging or whatnot then you have to change only one place. Code duplication should be avoided event for trivial things like that.

  2. You swallow exceptions in Hook and Unhook and return a bool which tells you nothing except that it went wrong. Same in OpenConnection and CloseConnetion. You throw away a lot of information which will be useful for troubleshooting should the need arise. In general I would consider that a bad idea.

  3. Replace your magic constant Thread.Sleep(100); either with a const definition or even a setting you can tweak. If you make it a setting please make it a TimeStamp - I personally find all that code which scatters around ints with implicit units very annoying. I often have to look up what it means because it's not always that obvious - seconds, milliseconds, ..?

Not sure if still relevant, but here you go:

  1. Consider refactoring your if (this.disposed) { } into a method like ThrowIfDisposed(). If you ever want to change your dispose method or add logging or whatnot then you have to change only one place. Code duplication should be avoided even for trivial things like that.

  2. You swallow exceptions in Hook and Unhook and return a bool which tells you nothing except that it went wrong. Same in OpenConnection and CloseConnetion. You throw away a lot of information which will be useful for troubleshooting should the need arise. In general I would consider that a bad idea.

  3. Replace your magic constant Thread.Sleep(100); either with a const definition or even a setting you can tweak. If you make it a setting please make it a TimeStamp - I personally find all that code which scatters around ints with implicit units very annoying. I often have to look up what it means because it's not always that obvious - seconds, milliseconds, ..?

Source Link
ChrisWue
  • 20.6k
  • 4
  • 43
  • 107

Not sure if still relevant, but here you go:

  1. Consider refactoring your if (this.disposed) { } into a method like ThrowIfDisposed(). If you ever want to change your dispose method or add logging or whatnot then you have to change only one place. Code duplication should be avoided event for trivial things like that.

  2. You swallow exceptions in Hook and Unhook and return a bool which tells you nothing except that it went wrong. Same in OpenConnection and CloseConnetion. You throw away a lot of information which will be useful for troubleshooting should the need arise. In general I would consider that a bad idea.

  3. Replace your magic constant Thread.Sleep(100); either with a const definition or even a setting you can tweak. If you make it a setting please make it a TimeStamp - I personally find all that code which scatters around ints with implicit units very annoying. I often have to look up what it means because it's not always that obvious - seconds, milliseconds, ..?