4
\$\begingroup\$

I am using a recursive directory scan to find all objects inside a Unity3D project. After that, I wish to move all these files to a sorted List. Is there a cleaner and/or more optimal way of doing so? I have the feeling the AssignFileToList could be less complex than it currently is.

//Requestable total values
public int 
    TotalDirectory,
    TotalFile,
    TotalMaterial,
    TotalTexture,
    TotalCSharp,
    TotalJava,
    TotalBoo;

//Returns the total sum of all script coutns
public int TotalScript
{
    get
    {
        return allCSharp.Count + allUnit.Count + allBoo.Count;
    }
}

//Request a List with the sorted materials (not relevant to my current question)
public List<Material> MaterialList(string list)
{
    if (list == "unique")
        return unique;
    else if (list == "broken")
        return broken;
    else if (list == "duplicate")
        return duplicate;
    else if (list == "dupof")
        return dupof;
    else
        return null;
}

#endregion

//Assorted Lists
private List<Material> allMat = new List<Material>();
private List<Texture> allTex = new List<Texture>();
private List<string> allCSharp = new List<string>();
private List<string> allBoo = new List<string>();
private List<string> allUnit = new List<string>();
private List<Shader> allShader = new List<Shader>();

//not sure if gonna be used in future, but is here anyway.
private List<GameObject> allPrefab = new List<GameObject>();

//After shader sorting (not relevant to my current question)
private List<Material> unique = new List<Material>();
private List<Material> broken = new List<Material>();
private List<Material> duplicate = new List<Material>();
private List<Material> dupof = new List<Material>();

public void ScanSubDir(string dir)
{
    //Count all directorys we reach and find all files inside of it.
    TotalDirectory++;
    FetchFilesFromDirectory(dir);

    //Find all subdirectories of dir
    List<string> subDir = new List<string>(Directory.GetDirectories(dir));
    if (subDir.Count > 0)
    {
        //Let each sub directory run through again to see if another sub directory exists within
        foreach (string subdir in subDir)
        {
            ScanSubDir(subdir);
        }
    }
}

void FetchFilesFromDirectory(string dir)
{
    //Get all files from provided directory
    List<string> files = new List<string>(Directory.GetFiles(dir));
    if (files.Count > 0)
    {
        //Count each file, and sort it accordingly
        foreach (string file in files)
        {
            TotalFile++;
            AssignFileToList(file);
        }
    }
}

void AssignFileToList(string file)
{
    //If it is not a meta file, continue and check types
    if (!file.Contains(".meta"))
    {
        //Remove the first part of the string so we can use internal pathfinder of unity
        file = file.Substring(file.IndexOf("Assets"));

        //If it is material, sort it to the material list.
        if (file.Contains(".mat"))
            allMat.Add((Material)AssetDatabase.LoadAssetAtPath(file, typeof(Material)));

        //If it is a supported format for textures, add it to the textures list
        else if (file.Contains(".psd") || file.Contains(".tiff") || file.Contains(".jpg") || file.Contains(".tga") || file.Contains(".png") || file.Contains(".gif") || file.Contains(".bmp") || file.Contains(".iff") || file.Contains(".pict"))
            allTex.Add((Texture2D)AssetDatabase.LoadAssetAtPath(file, typeof(Texture2D)));

        //If it is a shader, add it to the shader list
        else if (file.Contains(".shader"))
            allShader.Add((Shader)AssetDatabase.LoadAssetAtPath(file, typeof(Shader)));

        //Sort scripts for later use
        else if (file.Contains(".cs"))
            allCSharp.Add(file);
        else if (file.Contains(".js"))
            allUnit.Add(file);
        else if (file.Contains(".boo"))
            allBoo.Add(file);

        //Filter all Gameobjects to List
        else if (file.Contains(".prefab"))
            allPrefab.Add((GameObject)AssetDatabase.LoadAssetAtPath(file, typeof(GameObject))); ;
    }
}
\$\endgroup\$
  • \$\begingroup\$ It's not clear what you are asking. Example 1: if MaterialList() is not relevant for the question, why do you include it? Example 2: if the sorting code is not included, why do you mention the sorting several times? \$\endgroup\$ – ANeves wants peace for Monica Dec 29 '14 at 18:45
  • \$\begingroup\$ @ANeves This is due to the fact that the sorting code is highly involved in the next step of this code, after the initial sorting. As this is not fully functional yet this is not ready for reviewing. Beside's that I had the high feeling that the complexity in my assignfiletolist function was to high. And wanted to have that reviewed, and not the direct need to have my sorting function reviewed 'yet'. And as Aneves pointed out in his answer this certainly seems to be the case. The simplifying that is \$\endgroup\$ – MX D Dec 29 '14 at 19:55
  • \$\begingroup\$ A follow up question of this has been asked, including the functional sorting code proccessing-several-items-to-check-for-uniquity @ANeves \$\endgroup\$ – MX D Dec 30 '14 at 13:35
5
\$\begingroup\$

Two quick pointers:

Handling files by extension

You should use fileName.EndsWith(".cs") instead of fileName.Contains(".cs").
Otherwise, you will have problems when someone uses a valid but unexpected file name such as finished.material.cs.

Even better, you should extract the extension, and then switch on it:

switch(extension) {
    case "meta":
        return;
    // ...
    case "psd":
    case "tiff":
    case "jpg":
        allTex.Add((Texture2D)AssetDatabase.LoadAssetAtPath(file, typeof(Texture2D)));
        break;
    case "cs":
        allCSharp.Add(file);
        break;
    // ...
}

Use enums when options belong to a concrete set

For the method MaterialList method, you should receive an enum:

public enum MaterialListType {
    Unique,
    Broken,
    Duplicate,
}
public List<Material> MaterialList(MaterialListType type) {
    // ...
}

and then call it like List<Material> materials = GetMaterialsOfType(MaterialListType.Broken);.

And that is a terrible method name! Always use verbs in method names.

public List<Material> GetMaterialsOfType(MaterialListType type) {
    // ...
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Considering "extract the extension": wouldn't it be better to use the FileInfo class? \$\endgroup\$ – BCdotWEB Dec 30 '14 at 15:13
  • \$\begingroup\$ @BCdotNET yes, or use System.IO.Path's GetExtension method as suggested by RubberDuck. \$\endgroup\$ – ANeves wants peace for Monica Dec 30 '14 at 15:57
6
\$\begingroup\$

I'm going to reiterate ANeves's answer and suggest that you not use Contains() to determine what the file extension is, but I don't recommend using EndsWith() either. It would be much better to use System.IO.Path's GetExtension method. Doing so should save you a number of headaches down the road when you come across some oddly named directories. (Do use a switch though!)


I don't like public variables.

//Requestable total values
public int 
    TotalDirectory,
    TotalFile,
    TotalMaterial,
    TotalTexture,
    TotalCSharp,
    TotalJava,
    TotalBoo;

As it is, client code could set these variables and you'd be left searching for a bug in the wrong part of the code base. It would be better to create read only properties from these. For example:

private int directoryCount;
public int TotalDirectories { get {return directoryCount;} }

Now you're counts are safe from inadvertent changes by the client.

Other properties could just return the count from whichever list you're interested in.


I feel that your ScanSubDir method is a classic example of recursion and well done, but it seems that there's a lot of void going on. I try to avoid using void when ever possible. It tends to mean that the code is manipulating things at a larger scope than it should. Consider if your methods should be returning something instead. Perhaps a count of the directories or files found.


This should definitely be converted to an enum and a switch.

//Request a List with the sorted materials (not relevant to my current question)
public List<Material> MaterialList(string list)
{
    if (list == "unique")
        return unique;
    else if (list == "broken")
        return broken;
    else if (list == "duplicate")
        return duplicate;
    else if (list == "dupof")
        return dupof;
    else
        return null;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You could also do public int TotalDirectories { get; private set; }, instead of using a private backing field. \$\endgroup\$ – ANeves wants peace for Monica Dec 30 '14 at 14:58
  • \$\begingroup\$ Yes @ANeves you absolutely could. Personally, I probably would, but a lot of people don't care for the this keyword. \$\endgroup\$ – RubberDuck Dec 30 '14 at 15:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.