HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Simulate the "cd" command of a file system

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thefilesystemcommandsimulate

Problem

Write a function that provides change directory (cd) function for an
abstract file system.


Notes:



  • Root path is '/'.



  • Path separator is '/'.



  • Parent directory is addressable as "..".



  • Directory names consist only of English alphabet letters (A-Z and a-z).





For example, new Path("/a/b/c/d").cd("../x").getPath() should return
/a/b/c/x.


Note: The evaluation environment uses '\' as the path separator.

It works, but is there a better way to do this?

```
public class Path {
private String path;

public Path(String path) {
this.path = path;
}

public String getPath() {
return path;
}

public Path cd(String newPath) {

//1step consume newPath
//newPath to list
//take 1 char, head pull
//locate it with this.path,
// case / ../ ./
// this.path end poll
//return changed path and newPath

//recursive cd again

if( "".equals(this.path)||"".equals(newPath)||!this.path.startsWith("/"))
return this;
else if(newPath.startsWith("/")) {
this.path = "/";
return this.cd(newPath.length()>1?newPath.substring(1):"");

}
else{
if(newPath.startsWith("..."))
return this;
else if(newPath.startsWith("../")) {
this.cdup1();
return this.cd(newPath.length()>3?newPath.substring(3):"");
}else if(newPath.startsWith("./")) {
return this.cd(newPath.length()>2?newPath.substring(2):"");
}
else if(newPath.startsWith("..")) {
this.cdup1();
return this.cd(newPath.length()>2?newPath.substring(2):"");
}else if(newPath.startsWith(".")) {
return this.cd(newPath.length()>1?newPath.substring(1):"");
}
else {

String [] strarr = newPath.split("/");

Solution

Overcomplicated

The implementation is really overcomplicated and hard to read.


It works, but I feel there must be better way to do that?

Your instincts are good. When you know there's a better way, something you can try is start over from scratch using a different algorithm and see where that goes. Another technique is using TDD and let the tests drive your design.

Poor readability

The code is too compactly written at many places, for example:

if( "".equals(this.path)||"".equals(newPath)||!this.path.startsWith("/"))
    // ...

    return this.cd(newPath.length()>1?newPath.substring(1):"");
    // ...

    return this.cd(newPath.length()>3?newPath.substring(3):"");


This is really hard to read like that and doesn't help the understandability of the code.
All modern IDE's have an auto-reformat function that will put spaces around operators and make the code much easier to read.

Normalization

The cd method effectively normalizes the path, for example a/b/.. becomes correctly a. But the initial path passed to the constructor is never normalized. For example new Path("/a/b/../..").cd("c").getPath() returns /a/b/../../c instead of c.

This is a bit confusing inconsistency. It would be better to normalize the initial path too.

With a good normalizer method, the implementation of cd itself could be much simpler:

public Path cd(String newPath) {
    if (isAbsolutePath(newPath)) {
        path = normalizePath(newPath);
        return this;
    }

    path = normalizePath(path + SEPARATOR + newPath);
    return this;
}


In this case, of course, normalizePath does the real heavy lifting.
It could go something like this:

private String normalizePath(String path) {
    boolean isAbsolute = isAbsolutePath(path);

    List parts = new ArrayList<>();
    for (String part : path.split(SEPARATOR)) {
        if (part.isEmpty() || part.equals(".")) {
            continue;
        }
        if (part.equals("..")) {
            if (parts.isEmpty()) {
                if (isAbsolute) {
                    continue;
                }
            } else {
                if (!parts.get(parts.size() - 1).equals("..")) {
                    parts.remove(parts.size() - 1);
                    continue;
                }
            }
        }
        parts.add(part);
    }

    String prefix = isAbsolute ? SEPARATOR : "";
    return prefix + String.join(SEPARATOR, parts);
}


Bug or unclear specs?

What should new Path("a/b").cd("..").getPath() return?
I would expect a, but in fact it gives a/b.

What about new Path("a/b").cd("../../..").getPath()?
I would expect .., but it gives a/b for this too.

Special treatment of ...

There is a condition for path segments starting with .... Why is that? The description says that directory names contain only letters of the alphabet.
That makes this special treatment odd.
It seems that such path segment is illegal, and throwing an IllegalArgumentException would seem appropriate.

Testing

For something a bit tricky like this,
the main method in the class is really far too little to verify that the class works well.
Not to mention of course that testing in a main method is not a good way.
I strongly recommend to develop test cases and cover as many interesting corner cases as possible, to flush out obscure bugs.

Code Snippets

if( "".equals(this.path)||"".equals(newPath)||!this.path.startsWith("/"))
    // ...

    return this.cd(newPath.length()>1?newPath.substring(1):"");
    // ...

    return this.cd(newPath.length()>3?newPath.substring(3):"");
public Path cd(String newPath) {
    if (isAbsolutePath(newPath)) {
        path = normalizePath(newPath);
        return this;
    }

    path = normalizePath(path + SEPARATOR + newPath);
    return this;
}
private String normalizePath(String path) {
    boolean isAbsolute = isAbsolutePath(path);

    List<String> parts = new ArrayList<>();
    for (String part : path.split(SEPARATOR)) {
        if (part.isEmpty() || part.equals(".")) {
            continue;
        }
        if (part.equals("..")) {
            if (parts.isEmpty()) {
                if (isAbsolute) {
                    continue;
                }
            } else {
                if (!parts.get(parts.size() - 1).equals("..")) {
                    parts.remove(parts.size() - 1);
                    continue;
                }
            }
        }
        parts.add(part);
    }

    String prefix = isAbsolute ? SEPARATOR : "";
    return prefix + String.join(SEPARATOR, parts);
}

Context

StackExchange Code Review Q#132229, answer score: 7

Revisions (0)

No revisions yet.