Some code
What you do not like about this code ? ( I have found 3 things – how many can you find?)
01 02 03 04 05 06 07 08 09 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 | public static MyNewDocument OpenDocument(string FileDocPath , out int codeError){ if (string.IsNullOrEmpty(FileDocPath) || !File.Exists(FileDocPath)) { codeError = 1; return null; } MyNewDocument doc = null; try { doc = new MyNewDocument(FileDocPath); if (doc == null) throw new Exception(); codeError = 0; return doc; } catch { codeError = 2; return null; }} |
Destule:
– returnarea de erorr codes prin parametri out in loc de exceptii
– naming conventions in C# (FileDocPath pascal case in loc de camel case)
– inghite toate exceptiile in loc sa le prinda doar pe cele specifice
– arunca o exceptie nespecifica (throw new Exception())
– functie statica publica (greu de facut unit testing cu asa ceva)
– initializarea doc cu null inainte de a apela constructorul (initializare inutila)
– un constructor in C# nu va crea niciodata un obiect null (eventual va arunca o exceptie), deci acel if (doc == null) e inutil
1. FileDocPath should be respect the lower camel case notation.
2. It is not advisable to throw exceptions as part of the logical method flow.
– ridiculous name for the document class
– the declaration of the doc object should be moved into the try block, it is only used there
– doc can never be null in C# after new
– a numeric code error (use an Enum)
other issues that may be design flaws or design features:
– procedural style of coding: static method that uses out parameters to present state
– using File.Exists to check for the existence of the file, thus assuming knowledge of the internal workings of MyNewDocument
– same thing for empty path, the responsibility of getting a document should rest with the class
– inproper naming conventions
– number error codes
– checking to see if a doc is null after new? wth.
– invalid use of catch block. (if your not going to do anything with it, let it bubble up)
For me it was main 3 things:
– use of error code and of the exception
– invalid name of the function
– magic constants codes
Thank you for your answers!
We have this practice at the office where we code review all new code. So I have experience with stuff like that (also known as colleague bashing – it should be a national sport! )