Skip to main content
Code Review

Return to Answer

deleted 2 characters in body
Source Link
BenKoshy
  • 1k
  • 5
  • 11
added 48 characters in body
Source Link
BenKoshy
  • 1k
  • 5
  • 11
  • Personally I think it’s conceptually easier to test this code if you are returning a string – rather than a void: what if you decided to change the out put device from a console window to your printer – it would not be easy to change that. What if sometimes you wanted a console window and other times you wanted to send the same thing via text message? I guess it’s easier to separate what is being done vs how it is being done. E.g. Output key is the abstract concept but this could be done a number of different ways: (i) to the console, or (ii) through a voice synthesiser etc. I wrote some tests but then i threw it once it broke when i changed the API.
  • Personally I think it’s conceptually easier to test this code if you are returning a string – rather than a void: what if you decided to change the out put device from a console window to your printer – it would not be easy to change that. What if sometimes you wanted a console window and other times you wanted to send the same thing via text message? I guess it’s easier to separate what is being done vs how it is being done. E.g. Output key is the abstract concept but this could be done a number of different ways: (i) to the console, or (ii) through a voice synthesiser etc. I wrote some tests but then
  • Personally I think it’s conceptually easier to test this code if you are returning a string – rather than a void: what if you decided to change the out put device from a console window to your printer – it would not be easy to change that. What if sometimes you wanted a console window and other times you wanted to send the same thing via text message? I guess it’s easier to separate what is being done vs how it is being done. E.g. Output key is the abstract concept but this could be done a number of different ways: (i) to the console, or (ii) through a voice synthesiser etc. I wrote some tests but then i threw it once it broke when i changed the API.
Source Link
BenKoshy
  • 1k
  • 5
  • 11

Testing

  • Personally I think it’s conceptually easier to test this code if you are returning a string – rather than a void: what if you decided to change the out put device from a console window to your printer – it would not be easy to change that. What if sometimes you wanted a console window and other times you wanted to send the same thing via text message? I guess it’s easier to separate what is being done vs how it is being done. E.g. Output key is the abstract concept but this could be done a number of different ways: (i) to the console, or (ii) through a voice synthesiser etc. I wrote some tests but then

Naming

  • What is data? It’s harder to think of a more abstract name. I’ve changed this to keywords.
  • Personally I like to see the type along with the variable declaration.

Refactoring the code:

  • I simplified and extracted some methods.

  • The The HandleKeyInput and the HandleTabInput both seem to be doing conceptually similar things: a key is inputted and then a result is obtained. If it’s a tab then do X, but if it’s a key then do Y. There’s a common abstraction here that can be extracted.

  • What I want to do is to make the two methods: HandleTabInput and HandleKeyInput to look and be absolutely identical.

  • Data (renamed to keywords) can be a field so there’s no need to pass it in as a variable.

  • I want to make two of your methods to basically be the same. They’re kinda doing this same things. After some refactoring this is what I got:

     public void HandleTabInput(StringBuilder builder, ConsoleKeyInfo keyInput)
     {
     // Perform calculation
     string match = ExtractMatch(builder);
     // Alter the builder
     builder.Clear();
     builder.Append(match);
     // Print Results
     PrintMatch(match);
     }
    

And then this:

 private void HandleKeyInput(StringBuilder builder, ConsoleKeyInfo keyInput)
 {
 if (keyInput.Key == ConsoleKey.Backspace && builder.ToString().Length > 0)
 {
 // Perform Calculation (nothing here)
 // Alter the builder
 builder.Remove(builder.Length - 1, 1);
 // Print Results
 ClearCurrentLine(); 
 Console.Write(builder.ToString().Remove(builder.ToString().Length - 1));
 }
 else
 {
 // Perform calculation (nothing here)
 // Alter the Builder
 var key = keyInput.KeyChar;
 builder.Append(key);
 // Print Reuslts
 Console.Write(key);
 }
 }

We can abstract out the calculation, and then the printing. After some work this was the result. It’s starting to look pretty similar:

 private void HandleKeyInput(StringBuilder builder, ConsoleKeyInfo keyInput)
 {
 if (keyInput.Key == ConsoleKey.Backspace && builder.ToString().Length > 0)
 {
 Program.KeyInput backSpaceKey = new Program.KeyInput.BackspaceInput(builder, keywords);
 backSpaceKey.AlterBuilder();
 backSpaceKey.PrintResult(); 
 }
 else
 {
 KeyInput input = new KeyInput.StandardKeyInput(builder, keywords, keyInput.KeyChar);
 input.AlterBuilder();
 input.PrintResult();
 }
 }

You could probably tidy things up a little more.

The final Result:

 public void RunProgram()
 {
 StringBuilder builder = new StringBuilder();
 ConsoleKeyInfo capturedCharacter = Console.ReadKey(intercept: true);
 while (EnterIsNotThe(capturedCharacter))
 {
 KeyInput key = KeyInput.GetKey(builder, keywords, capturedCharacter);
 builder = key.UpdateBuilder();
 key.Print();
 capturedCharacter = Console.ReadKey(intercept: true);
 }
 Console.Write(capturedCharacter.KeyChar);
 }

Concluding Thoughts

  • It reads marginally better. Still If you want to reuse the code you’ll have to make changes, but at least it will be a little easier to do when the time comes.
  • Encapsulation could be a little better – typically speaking you want the KeyInput class to be instantiated via the factory only.

A link to see the full code.

lang-cs

AltStyle によって変換されたページ (->オリジナル) /