Dont do this at home

From Juneday education
Jump to: navigation, search

We have written this page to discuss bad behaviour. Not bad student behaviour, but rather bad teacher behaviour. We (teachers) need to be aware of that our code is treated as good code by oour students. This means that we need to be as careful as when writing code in projects, or probably more precise.

General

If true return true

Ever seen this?

  if (a>10) {
     return TRUE;
  } else {
     return FALSE;
  }

Is the above really simpler than the below

  return a>10;

Here's another similar example

  if (expr) {
     return TRUE;
  } else {
     return FALSE;
  }

Is the above really simpler than the below

  return expr;

A variant of misuse of the IF statement is to overly check for some condition and not use the result of the check (Java but same applies to any language):

  if (m==null) {
    return null;
  }
  return m;

In the above, you always return the value of the m reference. So, why check if it's null if you are going to return null anyway in that case? Use this instead: return m;. Wasn't that both clearer and easier?

Java

Scanner overuse

For some reason some authors and teachers have obsessions. One of the most popular and strong obsessions is the use of Scanner. One of the authors (Henrik!) used to (partly) be one of these teachers and he still ought to be tormented for that. "Shame on you Mr Henrik!"

Scanning a file and lines

Here's a very typical example of using a Scanner to read from a file:

  public static List<Member> readMemberFile(String fileName) throws MemberFileException, IOException {
    List<Member> members = new ArrayList<Member>();
    try {
      File file = new File(fileName);
      Scanner fs = new Scanner(file);
      while(fs.hasNext()) {
        String line = fs.nextLine();
        Scanner sc = new Scanner(line);
        sc.useDelimiter(",");
        String name = sc.next();
        String email = sc.next();
        int id = sc.nextInt();
        member.add(new Member(name, email, id));
      }
    } catch (Exception e) { ; }
    return members;
  }

Note: The code above is how a Scanner is often used. Note that in the loop a check is done to make sure there's a line ready to be read. But what about sc.next(), no check is done before calling next(). We're imitating the usual usage of Scanner. Don't use the code above...and most likely you don't need to use a Scanner either

Note: the code above is a rewrite of code from one of our videos on how to make things worse, called "Saa mycket saemre - part II" (Swedish) in the series "Så mycket sämre"(Swedish).


Why use a Scanner to split a string using a delimiter instead of using public String[] split(String regex)] in the String class (split) instead? That many lines, ok we could rewrite and loose some but still, to solve this trivial problem. Is it considered a non trivial problem by such authors?

Another solution (which is the code we used as a starting point (good code) when making things worse in the video above):

  public static List<Member> readMemberFile(String fileName) throws MemberFileException, IOException {
    return Files.lines(Paths.get(fileName))
      .map(s -> new Member(s.split(";")[0],
                           s.split(";")[1],
                           Integer.parseInt(s.split(";")[2])))
      .collect(Collectors.toList());
  }

Using Scanner as parameter in a constructor or method

Have you seen crap similar to this?

  public Member (Scanner s) {
    ....
  }

We have seen such code ("Fills me with the urge to defecate" (Waters, Bob Ezrin) The Trial, The Wall, Pink Floyd). Who on earth does this you may ask - well some authors and teachers we say. Why a Scanner object? Why open up for all possible kind of oddities in the argument someone supplies? What's "inside" the Scanner? So the constructor (or method in the class) above should know about the format in some file - or what ever the Scanner may "hide".

What to do instead? How about writing a dedicated class to read that specific file format? Here's an example:

public class MemberFileReader {
  public static List<Member> readMemberFile(String fileName) throws MemberFileException, IOException {
    return Files.lines(Paths.get(fileName))
      .map(s -> new Member(s.split(";")[0],
                           s.split(";")[1],
                           Integer.parseInt(s.split(";")[2])))
      .collect(Collectors.toList());
  }
}

Check out the entire file here: MemberCSVFileReader.java

Interactive constructor

  public Member() {
    Scanner sc = new Scanner(System.in);
    int id = sc.nextInt();
    String name = sc.next();
    String email = sc.next();
    this.name  = name;
    this.email = email;
    this.id    = id;
  }

So every time a Member object needs to be created the program goes interactive. Come on. Imagine if this Member class should be used in a "web system" - this means that when a user via a web interface fills in user data a system administrator need to interactively enter the same data. Is user interaction a thing we consider associated with a Member?

Note: the code above is a rewrite of code from one of our videos on how to make things worse, called "Saa mycket saemre - part II" (Swedish) in the series "Så mycket sämre"(Swedish).

We suggest writing a proper constructor and a separate class responsible for interacting awith a user. This class can have a method returning a Member instance.

Classes (constructors) knowing about the format of a file

We've seen code where the constructor of some class parses the input parameter (or even opens a file and parses the file). This obviously makes the class tightly coupled with the file format or syntax of the file, which isn't perfect.

Interactive constructors

We've even seen code where the constructor of some class is interactive. Instead of passing the constructor, say, a name and a phone number, the constructor runs code which prompts the user on standard out for the values, and reads them from standard in. That is so weird, that we don't know where to start.

Separate the user interaction from the construction of objects. Don't couple everything because doing so locks you and your code into one path which requires a lot of refactoring when stuff not related to the class itself changes. Some one said "Classes should have only one reason to change". That seems to make sense most of the time!

Interactive programs where the interaction is just about getting the data to operate on

We've seen countless examples on command line programs (in course literature and lectures) which start by asking the user interactively for the value needed to do the work. Use arguments instead of interaction!

Do you think e.g. ls, cp, cd, echo etc would have been a success and used daily by millions of people, if, say, ls when invoked started off by asking the user:

$ ls
LS: Good morning, Rikard. What directory or file do you want me to list?
home
LS: Can't find "home". Did you mean your home directory?
[y/n] y
The contents of /home/rikard is:
file1.txt
File.java
somedirectory/
$