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;
    }

}

Posted

2012-05-07

quiz

ignatandrei

Tags:

Comments

6 responses to “Some code”

Tudor

2012-05-07

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

Reply
Ovi

2012-05-07

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.

Reply
Siderite

2012-05-08

– 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

Reply
Matt

2012-05-13

– 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)

Reply
Andrei Ignat

2012-05-13

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!

Reply
Siderite

2012-05-15

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! )

Reply

Some code

Comments

6 responses to “Some code”

Leave a Reply Cancel reply