Conversation
| import com.tabgok.harvester.commands.CommandResult; | ||
|
|
||
|
|
||
| public abstract class Entity { |
There was a problem hiding this comment.
make this an interface. abstract classes are rarely what you want.
|
|
||
| private Machine(){} | ||
|
|
||
| public static Machine getMachine(){ |
There was a problem hiding this comment.
"getMachine()" sounds to me like there's just one. simply ".create()" is pretty conventional for something like this.
| * everything within a machine from Hardware to Filesystems to applications. | ||
| */ | ||
| public class Machine extends Entity { | ||
| private StringData MachineID; |
There was a problem hiding this comment.
a mutable ID on an object "smells" like we should rethink the object model here. it can force us to worry about all sorts of shit later on that we won't want to.
do you have a reason for having it be mutable?
There was a problem hiding this comment.
I'm not picking and choosing fields for what is immutable and what isn't, maybe I should. For instance, if a disk gets moved in/out of a machine, I'd expect the values to update accordingly -> but then again, I'd want a new disk and not the old one.
The machine is difficult though, because the ID is read from disk (there's a linux file for ID) so I can't set it in the constructor easily, at the moment.
| * everything within a machine from Hardware to Filesystems to applications. | ||
| */ | ||
| public class Machine extends Entity { | ||
| private StringData MachineID; |
There was a problem hiding this comment.
I don't know what StringData is, but if your ID isn't a String, it should probably be some interface MachineId which hides all the details.
There was a problem hiding this comment.
StringData{
final string value;
final string variable;
final timestamp data;
}
| * everything within a machine from Hardware to Filesystems to applications. | ||
| */ | ||
| public class Machine extends Entity { | ||
| private StringData MachineID; |
There was a problem hiding this comment.
s/MachineID/machineId/ (read, or at least skim, oracle's java style guide)
There was a problem hiding this comment.
Oracle's style guide has a large warning saying "hasn't been updated since 1999" (http://www.oracle.com/technetwork/java/codeconvtoc-136057.html). Should I look there or here: https://google.github.io/styleguide/javaguide.html or somewhere else?
| * | ||
| * @return Return the active mounted partition points on the machine | ||
| */ | ||
| public String[] getmountedFilesystemIDs(){ |
There was a problem hiding this comment.
there are really good arguments for always passing around immutable data structures, and very few good arguments for passing around mutable ones. This is a good example of why mutable data structures are problematic -- you have to make a copy of it to return it from a method, and (something you missed here) you have to worry about concurrency issues.
at google, for example, this would return an ImmutableSet, and mountedFilesystems would be an ImmutableSet, so we could just return it.
Two options I can think of here:
- Find a library with immutable data structures that you like and use that (the one we use is known publicly as guava)
- just return a Set<String> that is a copy of the member variable; nobody wants an array.
| * @param IDs Update this machine's mounted partitions | ||
| */ | ||
| public void updateMountedFilesystemIDs(Set<String> IDs){ | ||
| mountedFilesystems.addAll(IDs); |
There was a problem hiding this comment.
you are probably better off just making mountedFilesystems non-final, and saying mountedFilesystems = copySet(IDs);
or, continuing with the immutability conversation, if possible, don't let mountedFilesystems be changed for the life of the object.
| * | ||
| * @param IDs Update this machine's mounted partitions | ||
| */ | ||
| public void updateMountedFilesystemIDs(Set<String> IDs){ |
| * | ||
| * @param ID Set this machine's ID | ||
| */ | ||
| public void setID(StringData ID){ |
|
|
||
| import com.tabgok.harvester.commands.CommandResult; | ||
|
|
||
|
|
There was a problem hiding this comment.
the package name and the class name are really vague, which might be OK but I can't tell without javadoc here explaining what this is :P
There was a problem hiding this comment.
I think I am implementing something similar to futures...but I don't have enough experience with those to know. A "command" is a generic "executed something" and a CommandResult is a "executed something's return value"
modify more after reading up on immutability
2 participants
Continuuing with adding classes - This is the first commit from the "Entity" portion of the project, or the in-memory model of the Machine.