Is there a better OOP approach in the memory file system?
have implemented an in-memory filesystem - this is a design question from LeetCode. Is there a better OOP approach to represent files and directories? The current code uses a boolean flag isDirectory to distinguish between files and directories and I'm not confident that it is the best approach. See the code below. I'll also be happy to get any other comments about the code - readability, variable and method naming, etc.
class FileSystem {
Inode root;
public FileSystem()
{
root = new Inode("/", true);
}
public Listls(String path) {
Listlisting = new ArrayList<>();
String[] queries = path.substring(1).split("/");
Inode node = root;
for(String query: queries)
{
if(query.isEmpty()) // happens when we search root
break;
node = node.children.get(query);
if(node == null)
return listing;
}
if(node.isDirectory)
{
node.children.forEach((name, inode) -> {
listing.add(name);
});
}
else
{
listing.add(node.Name);
}
Collections.sort(listing);
return listing;
}
public void mkdir(String path)
{
String[] subdirs = path.substring(1).split("/");
Inode node = root;
for(String dir: subdirs)
{
if(dir.isEmpty()) // happens when we search root
break;
if(!node.children.containsKey(dir))
node.children.put(dir, new Inode(dir, true));
node = node.children.get(dir);
}
}
public void addContentToFile(String filePath, String content)
{
String[] subdirs = filePath.substring(1).split("/");
Inode node = root;
for(int i=0; i{
if(!node.children.containsKey(subdirs[i]))
{
if(i < subdirs> node.children.put(subdirs[i], new Inode(subdirs[i], true));
else
node.children.put(subdirs[i], new Inode(subdirs[i], false));
}
node = node.children.get(subdirs[i]);
}
node.fileContent.append(content);
}
public String readContentFromFile(String filePath)
{
String[] subdirs = filePath.substring(1).split("/");
Inode node = root;
for(String dir: subdirs)
{
node = node.children.get(dir);
}
return node.fileContent.toString();
}
}
// Trie data structure for files and directories
class Inode
{
public String Name;
public boolean isDirectory=true;
public StringBuilder fileContent=null;
public HashMapchildren = null;
public Inode(String name, boolean is_dir)
{
this.Name = name;
this.isDirectory = is_dir;
if(!is_dir)
fileContent = new StringBuilder();
if(is_dir && children == null)
children = new HashMap<>();
}
}
The implementation is easy to understand but the model can be improved.
Input validation
As said by @Martin Frank it's very important to validate the input for a file system. For example:
public void mkdir(String path){
if(path==null) {
throw new IllegalArgumentException("Input cannot be null");
}
// ...
}
Bug
String[] queries = path.substring(1).split("/");This line cuts the first letter of the path and then splits it. If you call mkdir("newFolder") the result is a folder called ewFolder. You can't assume that all the users will use an absolute path.
Duplicated code
Parsing the path is duplicated many times, this is an indicator that you should move the logic into its own method.
Polymorphism
Using a single class Inode to model a file and a directory has the following issues:
Why a directory has fileContent?
Why a file has children?
How to add another entity like a symbolic link?
The class is going to became too complex and hard to test. Create multiple classes that extend Inode.
Encapsulation
The instance variables (fileContent, children, etc.) of Inode shouldn't be public. Consider these points:
What if you want to change the children to a LinkedList?
What if you want to store an image instead of text?
You'll need to change almost every methods of FileSystem! To fix this issue make the state of Inode private and provide access only via methods (Encapsulation).
Consider this design:
abstract class Inode {
private String name;
public Inode(String name) {
this.name = name;
}
public abstract boolean isDirectory();
// methods to get the name, rename, etc..
}
These are the classes File and Directory.
class File extends Inode {
public File(String name) {
super(name);
}
@Override
public boolean isDirectory() {
return false;
}
}
class Directory extends Inode {
private HashMap children;
public Directory(String name) {
super(name);
children = new HashMap<>();
}
@Override
public boolean isDirectory() {
return true;
}
public boolean contains(String nodeName) {
return children.containsKey(nodeName);
}
// methods to query children. Do not return the HashMap!
}In this way you can easily change the HashMap to another data structure without causing any impact in memory File System.
A file that contains text will extends from File:
class TextFile extends File{
private StringBuilder content;
public TextFile(String name) {
super(name);
content = new StringBuilder();
}
// methods to get and add content
}Separation of Concerns
The method ls does too many things:
Move the current directory to the given path -> move this logic in a method (e.g. cd)
List the files -> this is the purpose of the method
Sort the files -> don't assume that a user always wants a sorted output, this can be a parameter
Same for the methods addContentToFile and readContentFromFile, they are too high-level for a file system. Usually a file system provides only low-level methods to manage files like create/delete and write/read bytes.
You can move the high-level features from the file system to other classes.