Code Review: FileManager

[Thorarin]

A couple of things about the FileManager class:

• The class is public, but all methods are internal, so the class might as well be internal.
• All methods in the class are static. Make the class static
• The File.Exists ? ... : ... pattern leads to unneeded repetition. Use File.Open instead.
• Make use of the using statement to ensure streams/files are being closed under exceptional circumstances.
• Last but not least: the file format is binary, but the files have an .xml extension and the methods are named as if XML is being used.

Those are the most important issues, I would say. There are a few less important ones I'd still like to mention:

• Use constants for file names to avoid inconsistencies between loading and saving
• The method names feel 'asymmetrical'. If there is a SaveFoo method, I expect a LoadFoo method, not GetFoo or GetAllFoo.
• Some Lists are being created unnecessarily while loading data (if the file has any contents).

[jwood803]

I like these recommendations. Would an interface for the class help as well?

[Thorarin]

For FileManager? Before you can use an interface, you'd first have to refactor things to use an instance rather than static methods of course.

Locally I've made some changes which effectively made the whole FileManager class disappear altogether, creating a simple repository pattern for all the file access. Each repository is accessed through an interface. I'm not able to push anything to GitHub though and I'm not really sure why.

[jwood803]

Ahhh, I'll just wait for your changes and see how you did things. Sounds like the repository pattern is what I had in mind to use, as well. Thanks!