Hello Sebastian. I'm not sure why you directed feedback at /u/finkrer, but since I developed ironmunge I'll be responding to your questions.
I should be upfront about my reasons for responding. I'm not writing this out as any serious attempt to defend the project, but as an open letter to show how not to behave in the FOSS world. Because as open-source contributors, we should know that the currency of technical discussion is in contribution.
Good-faith efforts improve software through focused issues, pull requests, forks, and actual labor. Dissing codebases on Reddit by complaining about var do not. It has about as much impact as a guy on a subway screaming that the CIA has tapped into everyone's brains and that we need to wear tin-foil hats: he's probably wrong, he's definitely acting like a wad, and even if he was right, nothing is getting changed from what he's doing.
Since your top comment is already hidden, I don't think there's much to add on Reddit either. I've addressed the points of discussion from your follow-up comment here, and I hope it will provide a learning experience for anyone interested in C# and how ironmunge was developed.
tl;dr: Your technical points are wrong and your attitude is toxic. People who behave like you aren't contributing, but simply make it exhausting to be a FOSS dev. Be better.
i know the language well enough i guess ;)
i just don't like the keyword "var",
var is just shorthand for a variable whose type can be inferred from its declaration. It's used frequently in ironmunge as a stylistic choice for declarations where the type is obvious from its usage. This is both my personal preference, and it's the official convention.
It's a common misconception that var is some form of late binding or dynamic. It's not. The IL emitted is identical when using var vs. by specifying the type. I think part of the confusion is because of the difference in behavior compared to the same keyword in JavaScript.
i don't like that it's just a cheap wrapper for git
This is actually a good time to mention the development history of ironmunge.
ironmunge started out as an automated way to take save backups the way any CK2 player does. Copy your save file, rename it, repeat.
This is simple and works, but has some major issues:
- Save-file bloat will make it impractical to keep hundreds of ~40mb saves lying around
- Metadata is opaque and requires a separate database or index to track which save is which, for restoration
After this initial version, I started looking into ways to fix the bloat issue and reduce the amount of space used. My first try was repacking saves into a shared LZMA archive, but this was too slow, both for saves and restores.
I noticed that saves taken in quick succession were almost identical despite being similar in size. Two 40MiB saves might only have a few bytes of counts and checksum changed, but with the dumb copy system they would still take up 80MiB. In order to save only the changes between saves, I had ironmunge unpack each save and take a delta of it and the prior save through bsdiff/deltaq. This was even slower than LZMA, but resulted in delta chains that could represent tens of saves in just KiB.
The problem with this delta system was because it had a depth of the entire save history, saves would continually get slower, and restores at an arbitrary point in time were slow and complicated to rebuild.
Now, what if there were a tool that:
- is specifically designed to handle small deltas in large text files
- can guarantee portability across machines and is resilient to losing data from user error
- can quickly compute deltas and restore from arbitrary points in time
- can attach metadata and labels to saves, to find and restore them later
- is cross-platform with Windows, Mac, and Linux
- and most importantly... is already written, robust, and tested!
Tada. Now you're using git.
This still involved writing an entire .NET Standard library to act as a wrapper and parser for git, called corgit. This is because libgit2-based solutions do not have any packfile support, so git gc and other space-saving measures were impossible -- leaving us with save-file bloat again.
History lesson done. Moving on...
and the unordered structure... there's classes inside files named otherwise
The only instance of doubled-up classes within the same file that I can recall are the CommandLineParser.Option definitions at the head of each console app's main method.
I'm sure for pedantry's sake they could be broken out, but it would be mostly useless since they are:
- Used immediately after their definition
- Only used in that place
- Located logically adjacent to their purpose (console app arguments)
and in other parts you create an extra file for such a class,
Yes, utility classes (FileUtilities, SoundUtilities, ConsoleHelpers) and stateful classes (SaveHistory, ChangeMonitoring) have their own files. That's literally everything else -- Options are the only ones that are deliberately not standalone, because they're a single-use dumb bucket for argument values.
the order of variables and functions has no ORDER at all,
Generally, variable declarations are done in the default VS style: var for inline declarations, declarations close to usage, and with camelCasing for local scope identifiers.
Member layout tends to be in top-down order of "likelihood to change" or in logical order. This places const, readonly, and static members at the top, followed by ctor's, and regular members in order of definition-before-usage.
There's no guarantee that this will be 100% consistent, but it's the general spirit of the code style used.
you use static variables without setter and assign them when you could use const which would be evaluated at compile time instead
(rules here: const if never changed, readonly if initialized in constructor and never changed again, if you can't make it one of those you will definitely need a setter at one point and that one in a lot of cases is private)
There are no static fields in this solution. There are exactly three static properties in use. None of them will have setters, as they are either for value retrieval only, or they are deliberately immutable.
Using const is not correct, since that would imply that these values never change. Two of these are retrievers for paths--which would be nonsensical to use a const value for--and the last is static readonly because it is a prefix that could change at a future date. Since const strings are destructively inlined by the C# compiler (cf. Marc Gravell's answer here), it is a mistake to release code that others could use if it contains public const fields that may change in the future.
you retry copying a file with cancellation tokens
Yes, and this is one of the main features of ironmunge.
A major problem with long running CK2 games is save-file bloat. Particularly in late-game, or with a slow disk, it can take several seconds to complete a full save.
If we rely on FileSystemWatcher, there's some problems:
- On Created, saves won't be finished writing, and trying to copy them will give us a file in use exception
- On Changed, there's no guarantee that the file is finished writing either, since FSW can fire any number of Changed events without a way to tell when the operation is complete
There are much more invasive options for finding out when the save is finished, like hooking CK2's save method or even attaching a hook to ZwUnlockFile and monitoring handles system-wide.
These solutions are possible, but they're not safe, cross-platform, or simple.
You know what is? Retrying with exponential backoff.
variable names don't mean anything (e.g. "MaximumWait" because you multiply it by 3 for some reason? - WTF)
MaximumWait is the maximum wait time for exponential backoff, not a timeout. This is pretty clear from the comments and the usage.
the Dispose() in "ChangeMonitoring" makes anything but sense (the watcher can't be disposed twice anyways and locks internally; use a semaphore instead if you insist on something like this, but that code won't prevent a race-condition even if it was possible)
Sure, it can't be disposed twice as it's written now. But by that logic, since the application exits immediately after ChangeMonitoring goes out of scope, we could also not dispose anything and just rely on the process teardown. If we were lazy and did that, it would:
- Introduce bugs if anyone else extended or remixed this code without being aware of the active FSW
- Break our own assumptions if the flow of the app changes from how it's currently implemented in
Program.cs
That's not good praxis. Instead, Dispose is implemented using the Disposable pattern, and ChangeMonitoring, being a stateful class, is responsible for cleaning up anything it uses once they're no longer needed. That's good praxis.
that timespan thing at the beginning of FileUtilities should be const as well (at least the numbers and shit)
const has to be a compile-time constant. You can't have a compile-time constant with a TimeSpan.
The numbers are self-documenting from the usage -- they're waits used in an exponential backoff algorithm. Which is called out many times, both in code and comments:
catch (IOException)
{
//exponential backoff: 0 -> 1 -> 2 -> 4 -> 8 ...
waitSeconds = waitSeconds == 0 ? 1 : waitSeconds * 2;
}
var wait = GetWait();
//if wait is higher than maximumWait, wait for maximumWait
wait = wait > maximumWait ? maximumWait : wait;
//report how long we're waiting
progress?.Report(wait);
//wait and retry
await Task.Delay(wait, token);there's no consistency with modifiers (sometimes you write public/private/protected, sometimes you don't [just one example of inconsistency]),
Some places do use default modifiers rather than write them out for redundancy's sake. This is only a stylistic choice, since they are only omitted when the default is the correct modifier. If there's a place where omitting the modifier makes it unclear how the type or member can be referenced, please feel free to submit a PR to add in the explicit form.
you create variables where none are needed because the same behaviour could be achieved easier,
This is too vague to be useful. If you see a way to do accomplish the same task easier, please feel free to open a PR.
what on gods green earth is that "ownsSave" in ChangeMonitoring???
ownsSave uses a ConcurrentDictionary as a form of lock-free synchronization to ensure that successive saves of the same game are only handled once, rather than creating malformed or duplicate timeline entries.
do you want me to go on? i'm not trying to be mean here at all and i'm really sorry if it looks like that... i'm trying to point out things so you can improve on them
Your information is wrong. Please take a closer look at codebases before offering suggestions, or better yet, fork them and try your modifications yourself so that you can find out why a change would be needed.
you could make such great use of object orientation, yet you are coding mostly procedural and merely use objects as data you send over and classes just as namespaces to not have all functions in one file
I would be more horrified at any way two standalone console utilities could be contorted into an OOP representation. Are we trying to live up to the stereotype of Java and .NET developers being mindless OO drones?
I think that about covers it. If you've made it this far: thanks, and have a good time playing CK2! For everyone interested in .NET, C#, and ironmunge: I'm always open to development discussions in Discord and contributions on Github.
-JZ