public bool isLocked(string s) { if (!File.Exists(s)) return false; try { FileStream f = File.Open(s, FileMode.Open, FileAccess.ReadWrite); f.Close(); } catch { return true; } return false; }
Objections and Comments
The code abve has in my eyes several problems:
- Method name starts with lower case
- The name of the input parameter is meaningless
- Logical code blocks are not separated by a new line
- You need some time to understand what this 4 lines of code do
- Multiple instructions on the same line
- The "return false;" statement should be inside the try block because it is logically associated to it
- Missing Method documentation
Proposed Refactoring
/// <summary> /// Checks if a file is in use or not. /// </summary> /// <param name="filePath">The path to the file that should be checked.</param> /// <returns>True if the file is locked; false otherwise</returns> public bool IsLocked(string filePath) { if (!File.Exists(filePath)) { return false; } try { FileStream fileStream = File.Open(filePath, FileMode.Open, FileAccess.ReadWrite); fileStream.Close(); return false; } catch { return true; } }