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; } }
2 comments:
Hi Manfred
Nice.
But I disagree with 6. and 7.
>>6. The "return false;" statement should be inside the try block because it is logically associated to it
Disagree, and you as well ;-)
At least you didn't change your code...
>>7. Missing Method documentation
What is the value of this documentation?
What about changing the method name to: IsInUse or FileIsLocked. Is this method part of the File class?
If this a public API, maybe keep the doco...
My problem with this:
As soon as you refactor this method and you change a little meaning of it, you have to change the code comment as well, which is forgotten most of the times...
important is that works and not how it looks. some programmers make good looking code, but it works like nothing. that smells!
Post a Comment