Some code
What you do not like about this code ? ( I have found 3 things – how many can you find?)
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! )