#114 Add filesystem

[Raugharr]

No description provided.

[Raugharr]

Currently this method is called but the output is never used. I'm planning eventually to use this as a way to checksum files the client has to ensure they have the most up to date version instead of using the current datetime method.

[sandysimonton]

I love the trust you have in yourself! I would put something like this in and forget that it's there lol

[Raugharr]

Technically this PR is a split off of the work I wanted to do to use this. I also realized it might be a good idea to start talking about what I'm thinking of doing, so more context is here: #127

[sandysimonton]

Since this is intended to be a Singleton, it would be good to make the constructor private and call a static getInstance() method on FileSystem when FileSystem is needed, so this part goes away

[Raugharr]

I think its a good idea to make this a singleton, the only worry I have is on the client side the config directory is unknown until the connection is made (config is located at /data/servers/#{server_ip}.#{server_port}). I'd dislike having to check everytime you use the config dir to make sure you actually have one, but as long as you only use the getConfigDir method after your connected I don't think you'd run into any problems.

[sandysimonton]

Thinking in terms of future integrations with a database, I've found that a lot of fields marked final end up losing their final keyword because they're at their heart values configured elsewhere and retrieved from the database.

[Raugharr]

Technically its already happened, the serverconfig.txt file allows you to change this via the CAMPAIGNCONFIG command, but the name is hard coded on the client side IIRC.

[sandysimonton]

I wrote an MD5Hexer interface that did MD5, want to move that functionality over to IOUtil and delete the MD5Hexer file?

Want to explore adding a dependency that is well tested and already does these things?

[Raugharr]

What dependency did you have in mind? I would have thought using the MessageDigest would be robust enough since it is in the Java core library.

[sandysimonton]

Oh I did not realize that... my MD5 stuff I copied from a file I had written a while ago that I knew worked... I will note that down to see if I can change MD5Hexer over to MessageDigest and put it in IOUtil

[sandysimonton]

It is probably time to add org.apache.commons.commons.lang3 as a dependency

[sandysimonton]

I predict race conditions and data corruption on deployments where the client and the server are running on the same machine, if writing and reading to the same configuration locations. I think if server's configuration directory is different than the clients that we might be ok, but I also know a lot of convenience has been written into the code regarding shared files between client and server. Thoughts?

[Raugharr]

The OpList.txt file is weird. Its a file that contains a mini Operation class containing information for client use only. However the file is created and saved on the server side, then sent over the write to be saved on the client in data/server/#{server_dir}/OpList.txt. So we should be fine here if the client and server are on the same system. I think it might also be a good idea in the future to just have the OpList.txt in memory on the server since its only purpose is to be sent to clients.

[sandysimonton]

Don't want to hold a reference to fileSystem

[sandysimonton]

These classes all make a case for Spring core to be added as a dependency... they're all singleton patterns with interwoven dependencies that rely on files in a config directory. Thoughts?

[Raugharr]

I'm not that familiar with Spring core so you might have to elaborate more. Personally I'd not like to add more dependencies than needed but I can see MekWars designed in a way to have many singleton classes with IIRC is what Spring boot is supposed to solve.

[sandysimonton]

Should probably move all this logic to an OperationDetails (or some other name) class. The OperationWriter should be a small class that writes the object, and not be worried about the logic that goes into preparing the information it needs to write

[Raugharr]

Thats probably the right idea, I think this class might go away since its only use is to serialize data into a file, for the server to then read the file and give it to the client. I think it would make more since to just have the server just serialize the data directly to the client.

[sandysimonton]

protected static final members are pretty confusing... the protected keyword suggests inheritance, but static members aren't inherited, although the protected keyword exposes them to both subclasses and package-peers. It seems to suggest a sub-global, mutable state, but with the final keyword, mutability is clearly not intended. FileSystem probably needs to make use of a FileSystemConstants class full of public static final constants

[Raugharr]

Would it be simpler to just make these public then? The intent is to allow this to be scoped to child classes since they are the only ones who should use it but strictly speaking its not internal state so there should be no issue making it public.

[sandysimonton]

I think at the very least some investigation into client and server running FileSystem actions on the same machine should be explored

[sandysimonton]

Let me know when I should approve this, I'm not sure I'll be able to tell or if Github will alert me!

[Raugharr]

Made a few changes and fixes as well as some comments I missed.

[sandysimonton]

Moving forward!