5
\$\begingroup\$

Currently I am looping through dependencies of objects, and I am trying to replace GUID (unity object ID's) inside the files. Right now I am using the following code, any suggestions for improvement ?

public class SendToThread
{
    public int localID = 0;
    public string oldID;
    public string newID;
    public List<string> dependencyAssetPath = new List<string>();
    public string applicationPath;
}

public void Charlie()
{
    //toUseData is a List<SendToThread>
    for (int i = 0 ; i < toUseData.Count; i++)
    {
        while (toUseData[i].dependencyAssetPath.Count >= 1)
        {
            string[] lines = File.ReadAllLines(toUseData[i].applicationPath + toUseData[i].dependencyAssetPath[0]);

            for (int e = 0; e < lines.Length; e++)
            {
                DebugVoid(lines.Length.ToString());
                lines[e].Replace(toUseData[i].oldID.ToString(), toUseData[i].newID.ToString());

                if (e == lines.Length - 1)
                {
                    DebugVoid(toUseData[i].oldID.ToString() + "  " + toUseData[i].newID.ToString());
                    File.WriteAllLines(toUseData[i].applicationPath + toUseData[i].dependencyAssetPath[i], lines);
                }
            }
            toUseData[i].dependencyAssetPath.RemoveAt(0);
        }
    }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Charlie may just be the worst method name I've ever seen. Normally, methods are verbs. They aren't really ever people. \$\endgroup\$ – Magus Oct 2 '14 at 14:38
  • \$\begingroup\$ @Magus Its the naming sense for threads we use here. Thread 1) alpha 2)beta 3) charlie. Charlie is a thread running independently from the unity engine, this to prevent unity engine freezes. So Its not a name, its phonetic alphabet ;) \$\endgroup\$ – MX D Oct 2 '14 at 14:40
  • 1
    \$\begingroup\$ Time to go Charlie up some GameObject. Sorry, but even if you mean that it should do something with the third thread, it's a horrible name, because it doesn't convey that. Even if you understand that it's the name of the third thread, then what? What is happening to it? I think I'll just go and Rhode Island. \$\endgroup\$ – Magus Oct 2 '14 at 14:43
  • \$\begingroup\$ Have you considered Parallel.For? You might give it a shot and see if there's any discernible performance increase. \$\endgroup\$ – Ian P Oct 2 '14 at 14:43
  • 5
    \$\begingroup\$ Alpha, Beta, Charlie? What? Shouldn't it be Alpha, Beta, Gamma or Alpha, Bravo, Charlie? \$\endgroup\$ – Nick Udell Oct 2 '14 at 15:33
3
\$\begingroup\$
  • don't call ToString () on Strings
  • the e == lines.Length - 1 should be outside of the loop
  • extract the processing of the toUseData to a separare method
  • I would avoid public fields. Use properties instead

To expand Nick Udell's answer which has been a good start

public void ReadAssets()
{
    //toUseData is a List<SendToThread>
    foreach (var toUse in toUseData)
    {
        foreach (var assetPath in toUse.DependencyAssetPath)
        {
            string[] lines = File.ReadAllLines(toUse.ApplicationPath + assetPath);

            for (int e = 0; e < lines.Length - 1; e++)
            {
                DebugVoid(lines.Length.ToString());
                lines[e].Replace(toUse.OldID, toUse.NewID);
            }

            DebugVoid(toUse.OldID + "  " + toUse.NewID);
            File.WriteAllLines(toUse.ApplicationPath + assetPath, lines);
        }
        toUse.dependencyAssetPath.Clear();
    }
}

We should also integrate the points already said. So let us break it into smaller methods. To process each SendToThread of the list we introduce a new method ProcessSingleItem().

private void ProcessSingleItem(SendToThread item)
{
    foreach (String assetPath in item.DependencyAssetPath)
    {
        ProcessSingleItem(item.ApplicationPath + assetPath, item.OldID, item.NewID);
    }
    item.DependencyAssetPath.Clear();
}  

to process each item of the item.DependencyAssetPath property we introduce an overloaded ProcessSingleItem().

private void ProcessSingleItem(String fileName, String oldId, String newId)
{
    string[] lines = File.ReadAllLines(fileName);

    File.WriteAllText(fileName, ProcessLines(lines, oldId, newId));
}  

To process the lines of the file we introduce a new method ProcessLines()

private String ProcessLines(IEnumerable<String> lines, String oldId, String newId)
{
    StringBuilder sb = new StringBuilder(2048);
    foreach (String line in lines)
    {
        sb.AppendLine(line.Replace(oldId, newId));
    }
    return sb.ToString();
}  

and reduce the former method Charlie, after we rename it to ProcessAssests, which may not the best name but better than Charlie, to

public void ProcessAssests()
{
    //toUseData is a List<SendToThread>
    foreach (SendToThread item in toUseData)
    {
        ProcessSingleItem(item);
    }
}

In this way our code is better to read and to maintain.

\$\endgroup\$
4
\$\begingroup\$

Building on what Heslacher and Magus have said:

You should not make publically accessible non-readonly collections. So I've converted those to readonly collections. It didn't appear to change how you were accessing them anyway.

I've also swapped your outermost for loop for a foreach, which makes the code slightly cleaner and I've swapped your while loop for a foreach, because it appears to just be iterating over the collection. However this assumes that

File.WriteAllLines(toUseData[i].applicationPath + toUseData[i].dependencyAssetPath[i], lines);

is a bug and should instead be

File.WriteAllLines(toUseData[i].applicationPath + toUseData[i].dependencyAssetPath[0], lines);

If not, then the while loop should be used.

Resulting version:

public class SendToThread
{
    private int localId = 0;
    public int LocalID
    {
        get{ return localId;}
        set{ localId = value;}
    }
    public string OldID {get; set;}
    public string NewID {get; set;}
    private readonly List<string> dependencyAssetPath = new List<string>();
    public List<string> DependencyAssetPath
    {
        get{ return dependencyAssetPath;}
    }
    public string ApplicationPath {get; set;}
}

public void ReadAssets()
{
    //toUseData is a List<SendToThread>
    foreach(var toUse in toUseData)
    {
        foreach(var assetPath in toUse.DependencyAssetPath)
        {
            string[] lines = File.ReadAllLines(toUse.ApplicationPath + assetPath);

            for (int e = 0; e < lines.Length-1; e++)
            {
                DebugVoid(lines.Length.ToString());
                lines[e].Replace(toUse.OldID, toUse.NewID);
            }

            DebugVoid(toUse.OldID + "  " + toUse.NewID);
            File.WriteAllLines(toUse.ApplicationPath + assetPath, lines);
        }
        toUse.dependencyAssetPath.Clear();
    }
}
\$\endgroup\$

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.