7
\$\begingroup\$

I'm applying for a mid level devloper position.

In order to progress to 2nd interview, you need to pass a technical test. One of the questions was to write a function to work out linux based octal file permissions.

How would you guys improve it, and why?

using System;
using System.Collections;
using System.Linq;

public class ReadWriteExecute
{
    public static int CalculatePermissionSum(string permString)
    {
        int octalPerm = 0;

        foreach (char permission in permString.ToArray())
        {
            switch (permission)
            {
                case 'r':
                    octalPerm += 4;
                    break;
                case 'w':
                    octalPerm += 2;
                    break;
                case 'x':
                    octalPerm += 1;
                    break;
                case '-':
                    octalPerm += 0;
                    break;
            }
        }
        return octalPerm;
    }

    public static string SymbolicToOctal(string permString)
    {
        string octalPerm = string.Empty;      
        for(int x=0; x<=6;x+=3)
        {

            octalPerm += CalculatePermissionSum(new string(permString.Skip(x).Take(3).ToArray())).ToString();
        }
        return octalPerm;
    }

    public static void Main(string[] args)
    {
        // Should write 752
        Console.WriteLine(ReadWriteExecute.SymbolicToOctal("rwx-x--r-"));
    }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Have you gotten feedback from your interviewers about your solution yet? \$\endgroup\$ – 200_success Feb 7 '18 at 4:05
6
\$\begingroup\$

My thoughts for improvement:

  1. You don't need to run ToArray() on your string to loop through it. Plus, it seems like CalculatePermissionSum() is expecting a single permission string like "rwx" or "r-x", so in a fixed-length situation like this (where the flags are also in a predefined order), don't bother with a loop.

    int octalPerm = 0;
    octalPerm += (permString[0] == 'r') ? 4 : 0;
    octalPerm += (permString[1] == 'w') ? 2 : 0;
    octalPerm += (permString[2] == 'x') ? 1 : 0;
    return octalPerm;
    
  2. SymbolicToOctal doesn't account for permission representations where the first character is a non-permission flag, like a directory "drwx-x--r-"

  3. There's no error-checking in case an unexpected value comes through. If I ran SymbolicToOctal("Hello World"), I should really get an exception of some kind.

  4. I disagree with the previous comment about using regular expressions to validate the input. Validation is good, but regular expressions would add a (relatively) huge amount of overhead for such simple validation. Code that works on file permissions isn't likely to be run a couple times here and there - it's more likely to be used as part of something that might run a LOT, so every inefficiency will add up. If you validate, just check the length and the expected individual characters. It might be more code compared to a regex, but it'll run a LOT faster.

  5. If I was an interviewer, I would give a LOT of brownie points to the interviewee who threw in some unit-testing code.

\$\endgroup\$
6
\$\begingroup\$

I find in general your implementation is a good start because it separates splitting the string into batches from calculating the octal value. You are not doing it in a single method.

Using a switch is also not such a bad idea here but it lacks the case-insensivity that should be achieved with char.ToUpperInvariant

If your application is unaffected by the current culture and depends on the case of a character changing in a predictable way, use the ToUpperInvariant method. The ToUpperInvariant method is equivalent to ToUpper(Char, CultureInfo.InvariantCulture).


You can push the separation of concerns a little bit further and extract two utility methods here.

The first one would be an extension that splits a collection into batches:

public static IEnumerable<IList<T>> Split<T>(this IEnumerable<T> source, int batchLength)
{
    var batch = new List<T>();
    foreach (var item in source)
    {
        batch.Add(item);
        if (batch.Count == batchLength)
        {
            yield return batch;
            batch = new List<T>();
        }
    }
}

and the second one would convert char into its octal value:

public static int ToOctal(this char value)
{
    switch (char.ToUpperInvariant(value))
    {
        case 'R': return 4;
        case 'W': return 2;
        case 'X': return 1;
        case '-': return 0;
        default: throw new ArgumentOutOfRangeException(paramName: nameof(value), message: "Value must be: R, W, X or -");
    }
}

Now you have two specialized methods that you can easily test.


You achieve the final result by combining the above two methods and LINQ into a third one:

public static IEnumerable<int> CalcPermissions(this string value)
{
    const int batchLength = 3;

    if (value.Length % batchLength != 0)
    {
        throw new ArgumentException(paramName: nameof(value), message: $"Value length must be divisible by {batchLength}.");
    }

    return
        from tripple in value.Split(batchLength)
        select tripple.Select(c => c.ToOctal()).Sum();
}

Three methods is what I would expect as an answer to this interview-question. Whether you should use a switch, a dictionary, regex or array indexing is an entirely different topic that would need proper benchmarks - if performance should be taken into account.

To me the most important thing is that you can separate a larger problem into smaller and testable ones. At this point it doesn't really matter how you implement the Split or ToOctal. As long as they are separate methods you can test other implementations later without changing the final result and without having to worry about breaking other functionalities because there is only one at a time you are working on.

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

I would not be impressed with this submission. Here are some of my notes:

  • Way too many data type conversations. You're converting from string to array and back several times.
  • No validation that the value is correct. I think you actually have an error in the permission string in your example. The format is well-known and you could use a regular expression.
  • Since you'll be validating with a regular expression, you can get rid of the skip(x) take(3) stuff. Just use Substring().
  • I wouldn't necessarily knock this point for an interview task, but I'd be more impressed with a separate class dealing with file permission strings:

    PermissionString permissionString = new PermissionString(permString);
    return permissionString.ToOctal();
    
\$\endgroup\$
1
\$\begingroup\$

First of all, you should add the "sticky bit's" S,s and t otherwise you'll fail parsing a permission like rwSrwxrwt.

Edit (17.05.2019):

According to this post (https://unix.stackexchange.com/questions/27250/uppercase-s-in-permissions-of-a-folder) and an personal encounter, here is the update.

  • S,s on the owner block is 4
  • S,s on the group block is 2
  • t on the public block is 1

After that, you could wrap all this in a single function like the following example. So you can simply convert a permission string like rwSrwxrwt to 5677 and vice versa.

    #region ConvertUnixChmod

    ///<summary>
    /// Convert an unix chmod (rwSrwxrwt) to an number (5677) and reverse
    ///<para/>*NOTE*
    ///<para/>This method is case-sensitive!
    ///</summary>
    ///<param name="chmod">(rwSrwxrwt) or (5677)</param>
    ///<returns><see cref="string"/></returns>
    public static string ConvertUnixChmod(string chmod)
    {
        char[][] pC = new char[3][];
        int[] pN = new int[] { 0, 0, 0, 0 };
        if (chmod.Length == 9)
        {
            pC[0] = chmod.Substring(0, 3).ToCharArray();
            pC[1] = chmod.Substring(3, 3).ToCharArray();
            pC[2] = chmod.Substring(6, 3).ToCharArray();
            for (int p = 0; p < 3; p++)
            {
                for (int i = 0; i < 3; i++)
                {
                    switch (pC[p][i])
                    {
                        case 'r':
                            pN[p + 1] += 4;
                            break;
                        case 'w':
                            pN[p + 1] += 2;
                            break;
                        case 'x':
                            pN[p + 1] += 1;
                            break;
                        case 's':
                            // Here we calculate the setuid/gid with "x"
                            pN[0] += 4 / (p + 1);
                            pN[p + 1] += 1;
                            break;
                        case 'S':
                            // Here we calculate the setuid/gid without "x"
                            pN[0] += 4 / (p + 1);
                            break;
                        case 't':
                            // Here we calculate the sticky bit
                            pN[0] += 1;
                            pN[p + 1] += 1;
                            break;
                    }
                }
            }
            return string.Concat(pN);
        }
        else if (chmod.Length == 4 && int.TryParse(chmod.Substring(0, 1), out pN[0]) && int.TryParse(chmod.Substring(1, 1), out pN[1]) && int.TryParse(chmod.Substring(2, 1), out pN[2]) && int.TryParse(chmod.Substring(3, 1), out pN[3]))
        {
            pC[0] = new char[] { '-', '-', '-' };
            pC[1] = new char[] { '-', '-', '-' };
            pC[2] = new char[] { '-', '-', '-' };
            for (int i = 0; i < 3; i++)
            {
                switch (pN[i + 1])
                {
                    case 1:
                        pC[i][2] = 'x';
                        break;
                    case 2:
                        pC[i][1] = 'w';
                        break;
                    case 3:
                        pC[i][1] = 'w';
                        pC[i][2] = 'x';
                        break;
                    case 4:
                        pC[i][0] = 'r';
                        break;
                    case 5:
                        pC[i][0] = 'r';
                        pC[i][2] = 'x';
                        break;
                    case 6:
                        pC[i][0] = 'r';
                        pC[i][1] = 'w';
                        break;
                    case 7:
                        pC[i][0] = 'r';
                        pC[i][1] = 'w';
                        pC[i][2] = 'x';
                        break;
                }

                // Here we set the setuid/gid and sticky bits

                if (i == 0 && (pN[0] == 7 || pN[0] == 6 || pN[0] == 5 || pN[0] == 4))
                {
                    pC[i][2] = pC[i][2] == '-' ? 'S' : 's';
                }
                else if (i == 1 && (pN[0] == 7 || pN[0] == 6 || pN[0] == 3 || pN[0] == 2))
                {
                    pC[i][2] = pC[i][2] == '-' ? 'S' : 's';
                }
                else if (i == 2 && (pN[0] == 7 || pN[0] == 5 || pN[0] == 3 || pN[0] == 1))
                {
                    pC[i][2] = 't';
                }
            }
            return string.Concat(string.Concat(pC[0]), string.Concat(pC[1]), string.Concat(pC[2]));
        }
        return string.Empty;
    }
    #endregion
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. If you think your answer can add something new, please highlight and explain the corresponding parts of your code. Otherwise see: Why are alternative solutions not welcome? \$\endgroup\$ – AlexV Mar 26 at 19:35

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.