trjhtr

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-"));
 

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.

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.

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();