Commenting Challenge Response Part Two

(See CommentingChallenge and CommentingChallengeResponse.)

At this point, the code was done. I made a final clean-up pass through the code to improve readability. I changed several variable names and removed the dictionary argument from the class constructor. (The class now creates its own dictionary.) I also reordered the methods.

The only thing I don't like about the code as it currently stands is that _moreCharsInStream is not always quite correct. If the stream is passed in empty, then _moreCharsInStream is true when it should be false. This is handled by the if statement in parseNameValuePair(), but it's a bit of ugliness I don't like. I could have handled it by making _moreCharsInStream a method that read and cached one character, but the extra complexity seemed unwarranted.

As per CommentingChallengeResponse, I'm going through and naming the refactorings used. My comments are in italics. -- RobertWatkins

Jim's used RenameVariable? a lot here, to improve intent. He's also used RemoveParameter? to get rid of the unnecessary constructor arg.

---

  import java.io.*;
  import java.util.*;

  /**
   * Like HttpUtils.parseQueryString, except that it never throws parse
   * exceptions.
   */
   public class QueryStringParser {

    private Dictionary _result = new Hashtable();
    private boolean _moreCharsInStream = true;
    private InputStream _stream;

    public QueryStringParser(InputStream stream) {
      _stream = stream;
    }

    public Dictionary parseArgs() throws IOException {
      while (_moreCharsInStream) {
        parseNameValuePair();
      }
      return _result;
    }

    private void parseNameValuePair() throws IOException {
      String name = readUpTo('=');
      String value = readUpTo('&');
      if (!name.equals("") || !value.equals("")) {
        _result.put(name, value);
      }
    }

    private String readUpTo(char boundaryCharacter) throws IOException {
      String word = "";

      char character = readCharacter();
      while(_moreCharsInStream) {
        if (character == boundaryCharacter) return word;
        else if (character == '%') word += readHexEncodedCharacter();
        else if (character == '+') word += " ";
        else word += character;
        character = readCharacter();
      }
      return word;
    }

    private String readHexEncodedCharacter() throws IOException {
      int sixteens = readHexDigit();
      int ones = readHexDigit();
      if ((sixteens < 0) || (ones < 0)) return "";

      char character = (char)((16 * sixteens) + ones);
      return "" + character;
    }

    private int readHexDigit() throws IOException {
      return Character.digit(readCharacter(), 16);
    }

    private char readCharacter() throws IOException {
      int character = _stream.read();
      if (character < 0) {
        _moreCharsInStream = false;
        return '\0';
      }
      else {
        return (char)character;
      }
    }

  }

---

But after looking at the code a bit more, I broke down and did the read-ahead. It actually simplified the code.

As Jim hinted above, he's going to ReplaceTempWithQuery. He's then used SubstituteAlgorithim? in the new temp and the readCharacter() method to RemoveConditional?.

---

  import java.io.*;
  import java.util.*;

  /**
   * Like HttpUtils.parseQueryString, except that it never throws parse
   * exceptions.
   */
   public class QueryStringParser {

    private Dictionary _result = new Hashtable();
    private InputStream _stream;
    private int _nextCharacter;

    public QueryStringParser(InputStream stream) throws IOException {
      _stream = stream;
      _nextCharacter = stream.read();
    }

    public Dictionary parseArgs() throws IOException {
      while (hasAnotherCharacter()) {
        parseNameValuePair();
      }
      return _result;
    }

    private void parseNameValuePair() throws IOException {
      String name = readUpTo('=');
      String value = readUpTo('&');
      _result.put(name, value);
    }

    private String readUpTo(char boundaryCharacter) throws IOException {
      String word = "";

      while(hasAnotherCharacter()) {
        char character = readCharacter();

        if (character == boundaryCharacter) return word;
        else if (character == '%') word += readHexEncodedCharacter();
        else if (character == '+') word += " ";
        else word += character;
      }
      return word;
    }

    private String readHexEncodedCharacter() throws IOException {
      int sixteens = readHexDigit();
      int ones = readHexDigit();
      if ((sixteens < 0) || (ones < 0)) return "";

      char character = (char)((16 * sixteens) + ones);
      return "" + character;
    }

    private int readHexDigit() throws IOException {
      if (!hasAnotherCharacter()) return -1;

      return Character.digit(readCharacter(), 16);
    }

    private boolean hasAnotherCharacter() {
      return (_nextCharacter >= 0);
    }

    private char readCharacter() throws IOException {
      if (!hasAnotherCharacter()) throw new IllegalStateException("assertion failed");

      char result = (char)_nextCharacter;
      _nextCharacter = _stream.read();
      return result;
    }

  }

---

And that's it.

-JimLittle

---

I like the new code but isn't refactoring supposed to be making small changes. What you've done here make one rather large change. -- GlenStampoultzis

It's a big change, but it's done through a series of little changes. That's what refactoring is meant to be about. It would have been nice if JimLittle had put in what refactorings he was applying at each step. He does have comments, but a nice WikiName label would be good. Hmm... I'll go put those in [done now, hope that helps] -- RobertWatkins

Thanks for your additions, Robert. Your assumption was correct -- to conserve space, each step was actually several refactorings. Unfortunately, I didn't keep any notes (other than the WikiPage itself) so I don't remember which refactorings I thought I was doing. But your guesses seem about right. :) --JimLittle

Yes, thanks Robert. Detailed refactorings can be a difficult thing to follow on paper/screen. A screen recording program would be more useful to follow what was going on here I think. Anyone know of any free ones? -- Glen Stampoultzis