Added OSProcess constructor with PID (#957) #960
Conversation
Potat0x
force-pushed
the
pid-process-constructor
branch
from
455fd7d to
39d8725
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
| case FREEBSD: | ||
| return new FreeBsdOperatingSystem(); | ||
| default: | ||
| return null; |
There was a problem hiding this comment.
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.
| SystemInfo si = new SystemInfo(); | ||
| OperatingSystem os = si.getOperatingSystem(); | ||
| OSProcess oldProcess = os.getProcess(os.getProcessId()); | ||
| int givenPid = oldProcess.getProcessID(); |
There was a problem hiding this comment.
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().)
There was a problem hiding this comment.
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).
|
In OperatingSystemTest.java I found always true conditions (inside assert): |
|
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.... ) |
|
Actually, after reviewing, I decided the best thing to do for an invalid PID was to throw an 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:
|
|
Why not all three? It should be good. |
|
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. |
* public OSProcess constructors now require OperatingSytem object * new OperatingSytem object is no longer created before each update
|
You are right, of course (I misread your post, sorry). Changes pushed. |
|
Looks great! You no longer needed the Letting the tests run and then I'll merge this! Thanks for your efforts! |
Fixes #957