Added OSProcess constructor with PID (#957)

[Potat0x]

Fixes #957

[coveralls]

Coverage increased (+0.8%) to 79.313% when pulling 4f1cf52 on Potat0x:pid-process-constructor into 87986bd on oshi:master.

[codecov-io]

Codecov Report

Merging #960 into master will increase coverage by 0.24%.
The diff coverage is 81.25%.

@@             Coverage Diff              @@
##             master     #960      +/-   ##
============================================
+ Coverage     75.14%   75.38%   +0.24%     
  Complexity        2        2              
============================================
  Files            22       22              
  Lines          1058     1105      +47     
  Branches        137      139       +2     
============================================
+ Hits            795      833      +38     
- Misses          204      210       +6     
- Partials         59       62       +3

Impacted Files	Coverage Δ	Complexity Δ
...core/src/main/java/oshi/software/os/OSProcess.java	89.51% <81.25%> (-4.24%)	0 <0> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e63b9ec...39d8725. Read the comment docs.

[codecov-io]

Codecov Report

Merging #960 into master will increase coverage by 0.24%.
The diff coverage is 81.25%.

@@             Coverage Diff              @@
##             master     #960      +/-   ##
============================================
+ Coverage     75.14%   75.38%   +0.24%     
  Complexity        2        2              
============================================
  Files            22       22              
  Lines          1058     1105      +47     
  Branches        137      139       +2     
============================================
+ Hits            795      833      +38     
- Misses          204      210       +6     
- Partials         59       62       +3

Impacted Files	Coverage Δ	Complexity Δ
...core/src/main/java/oshi/software/os/OSProcess.java	89.51% <81.25%> (-4.24%)	0 <0> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e63b9ec...de82460. Read the comment docs.

[dbwiddis]

This looks fantastic, thanks! I've made a few minor comments.

[dbwiddis]

This should never happen unless we add a new OS in the platform enum and don't implement it here, so it's probably safe to throw an exception here, and then you don't have to handle the null return value.

[dbwiddis]

You get major bonus points for writing a testcase!

I'm curious why store the result of getProcessID() in a variable unless you want to test against it... why not just store os.getProcessId() two lines up and then use it twice? (Or just not use a temp variable and only use os.getProcessId().)

[Potat0x]

Thanks :)

Good point, I change code to use temp variable twice (I think temporary variable is more meaningful and easier to read in this case).

[Potat0x]

In OperatingSystemTest.java I found always true conditions (inside assert):
if (onePid >= 0) { assertTrue(0 <= os.getChildProcesses(onePid, 0, null).length); }
There is two more similar assertions.

[dbwiddis]

OK, this looks great! I think the fact that the process name, path, etc. will be empty is sufficient testing material for a return value. I'll tweak the javadoc a bit for that and then merge. Thanks for your contribution!

(And thanks for pointing out those testcases... that one has always been problematic with race conditions.... )

[dbwiddis]

Actually, after reviewing, I decided the best thing to do for an invalid PID was to throw an InstantiationException so I added that code, updated the docs and the tests (and removed the always true ones you pointed out).

However, while running the testcase, it took a long time. I tracked it down to creating a new (platform specific) OperatingSystem object every time the update method was called, and all the overhead we get in a new OS (in Windows, multiple WMI calls, reading the registry, reading the event log...).

If this was only for creating the object I'd live with it, but the "update" method is intended to be one that a user will call regularly so we need to save that value somewhere, ideally passed as part of the constructor when the class is first created (required) rather than creating a new one ourselves. I'm thinking:

• private no-arg constructor, so we can rely on this field
• OSProcess(OperatingSystem os) constructor that sets the internal attribute, that we call from the other getProcess() code
• OSProcess(OperatingSystem os, int ProcessID) constructor that sets the internal attribute and updates for that PID, thus requiring the user to provide us the OS as an argument.

[Potat0x]

Why not all three? It should be good.

[dbwiddis]

If you don't let anyone call with no-args you can be sure the operating system field is populated on construction. If you leave it blank you have to create the object to store it, which is a (relatively) expensive operation and should be avoided. There's really no good reason to offer it.

[Potat0x]

You are right, of course (I misread your post, sorry). Changes pushed.

[dbwiddis]

Looks great! You no longer needed the getCurrentOperatingSystem() method so I deleted it -- but then it complained about not initializing operatingSystem in the non-used constructor so I deleted that too! We don't need it -- code trying to invoke it doesn't even compile.

Letting the tests run and then I'll merge this! Thanks for your efforts!