Code snippet



  • An anonymized piece of code I just came across.

    public bool GetInfoFromFile(string filename)
    {
    bool retVal = false;
    BinaryReader br = null;
    try
    {
    if (File.Exists(filename))
    {
    br = new BinaryReader(File.Open(filename,FileMode.Open,FileAccess.Read));
    /* snip - open file and get info*/
    }
    }
    catch (Exception ex)
    {
    throw;
    }
    finally
    {
    if (br != null)
    br.Close();
    }
    return retVal;
    }

     

    And to top it off, the one place this function is called, it doesn't even use the returned value.


  • :belt_onion:

    My first reaction was: "I suppose the developer didn't know you can have a finally block without a catch block..." but then I realized the function isn't actually returning any information about the file.

    But still there is a possibilty that "retval=true" is done in the part that is snipped. Maybe the "file information" is written to some member variables as well? Not a good practice but not easy to determine without knowing what was snipped.



  • sorry, should have made that clear.  the file information is written to member variables and used. 

     the retval is never touched.


  • :belt_onion:

    @campkev said:

    the file information is written to member variables and used. 

     the retval is never touched.

    Tell-tale signs of a badly designed class.

    In my opinion, a public method called Get<something> should have <something> as the return type. If it can't correctly do what it says on the can, it should throw an exception. As this function doesn't return anything and only modifies internal data members, it probably should be renamed to Load... or Init... Access modifier should be private as an external function probably has no business changing the internal state of the object in this way.

    The fact that "retval=true" isn't done, isn't the biggest WTF for me :-)



  •  @bjolling said:

    In my opinion, a public method called Get<something> should have <something> as the return type. If it can't correctly do what it says on the can, it should throw an exception.

     So GetLength should have Length as return type? I'd prefer int :P



  • @bjolling said:

    Tell-tale signs of a badly designed class.
    Looks to me like the class was written by someone who usually writes C, although it might also have been a PHP coder – after noticing he can't return false in a method suppposed to return a string he just did it with member variables instead of null.



  • @bjolling said:

    In my opinion, a public method called Get<something> should have <something> as the return type.

    Agreed. Variables and functions should always be given descriptive names. That function should be called GetBool.

    if(GetBool()!=False) myString = GetString(4);


  • :belt_onion:

    @joemck said:

    @bjolling said:

    In my opinion, a public method called Get<something> should have <something> as the return type.

    Agreed. Variables and functions should always be given descriptive names. That function should be called GetBool.

    if(GetBool()!=False) myString = GetString(4);

     

    Let me re-phrase that:

    a public method called Get<something> should return have typeof<something> as the return type.

    e.g. public String GetName( parameters );  //because String is the typeof Name



  • @bjolling said:

    @joemck said:

    @bjolling said:

    In my opinion, a public method called Get<something> should have <something> as the return type.

    Agreed. Variables and functions should always be given descriptive names. That function should be called GetBool.

    if(GetBool()!=False) myString = GetString(4);

     

    Let me re-phrase that:

    a public method called Get<something> should return have typeof<something> as the return type.

    e.g. public String GetName( parameters );  //because String is the typeof Name

    I once had a teammate in college who didn't quite get the concept of getters/setters. One example:

    public void getName(String name) {

             this.name = name;

    }

    public String setName() {

             return this.name;

    }

    The sad thing is that this guy had better coding practices than most of our colleagues, and he wasn't even a CS student.


Log in to reply