patternjavaModerate
Simple Self-Learning AI
Viewed 0 times
learningsimpleself
Problem
This is a programming challenge I set for myself a while back to create an AI that starts with no knowledge of anything whatsoever, and learns as you talk to it. (It can learn stuff like your name, how to say hello, goodbye, etc.)
I tried to use the least amount of hard-coded responses as possible. I'm happy that it works, but I feel like the code behind it is very clumsy and... just not very good. The program is called
Genesis.java
```
package genesis;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintStream;
import java.io.PrintWriter;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
public final class Genesis {
public static final String name = "Genesis";
public static final String version = "1.0";
public static void main(String[] args) {
try {
new Genesis();
} catch(IOException e) {
printError(new PrintWriter(System.err), e);
}
}
private final FileManager fm;
protected String last = null;
public Genesis() throws IOException {
log("Initializing " + toString() + "...");
log("Generating files...");
fm = new FileManager(this);
log(toString() + " started on " + System.getProperty("os.name"));
start();
stop();
}
public void stop() {
stop(0);
}
public void stop(int error) {
if(error == 0)
log(toString() + " shut down successfully!");
else
log(toString() + " shut down with error code: " + error);
if(fm != null)
fm.close();
System.exit(error);
}
public void start() {
try(BufferedReader r = new BufferedReader(new InputStreamReader(System.in))) {
System.out.print("You: ");
String s = r.readLine();
if(respond(s))
start();
} catch(Throwable t) {
printError(t);
}
}
public boolean respond(String s) { //decide how and what to respond, return if we should keep the program alive
if(s.trim().equals(""))
I tried to use the least amount of hard-coded responses as possible. I'm happy that it works, but I feel like the code behind it is very clumsy and... just not very good. The program is called
Genesis. Tell me what you think!Genesis.java
```
package genesis;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintStream;
import java.io.PrintWriter;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
public final class Genesis {
public static final String name = "Genesis";
public static final String version = "1.0";
public static void main(String[] args) {
try {
new Genesis();
} catch(IOException e) {
printError(new PrintWriter(System.err), e);
}
}
private final FileManager fm;
protected String last = null;
public Genesis() throws IOException {
log("Initializing " + toString() + "...");
log("Generating files...");
fm = new FileManager(this);
log(toString() + " started on " + System.getProperty("os.name"));
start();
stop();
}
public void stop() {
stop(0);
}
public void stop(int error) {
if(error == 0)
log(toString() + " shut down successfully!");
else
log(toString() + " shut down with error code: " + error);
if(fm != null)
fm.close();
System.exit(error);
}
public void start() {
try(BufferedReader r = new BufferedReader(new InputStreamReader(System.in))) {
System.out.print("You: ");
String s = r.readLine();
if(respond(s))
start();
} catch(Throwable t) {
printError(t);
}
}
public boolean respond(String s) { //decide how and what to respond, return if we should keep the program alive
if(s.trim().equals(""))
Solution
Genesis
-
It's best practices to let statements like
-
-
I'd rename
-
The naming in conjunction with the inverse logic of:
doesn't make your code clearer. I'd reverse the logic to:
The same applies to
-
Going through the class from the top raised the following questions:
-
Though using the l-value of assignments is valid it doesn't make your code clearer:
Sure, it gives you the opportunity to omit the braces for the preceding
-
prints just:
An explaining message would be nice for the users of your program.
-
I'd rename
-
The
can be simplified to:
-
If a read
-
FileManager
-
If I read a class name
-
I'd separate logging and response file handling combined in
-
Variable names like
I'd call them
-
I'd rename
-
You can shorten:
to:
using the diamond operator.
-
Why are you using Unicode's 65533D as
-
It's not recommended to omit braces in one-line blocks like:
-
I'd use:
instead of string concatenation:
Such you also probably would have recognized easier that there is a
-
I'd make variables with just two expected values like
ResponseType
-
Why do you want to get an enum constant by a string
-
I'd implement the enum with a constructor:
Running Genesis
-
for the first time from within Eclipse Luna:
I have my doubts about the second to last sentence. It is related to Genesis. Always! Since it happened when running it. Your end user doe
-
It's best practices to let statements like
if, for, try ... be followed by a space to disinguish them from method invocations. -
protected String last = null;protectedis pointless sinceGenesisis afinalclass and hence can't be extended.
- This variable holds the last what? Advancing in the code it's apparently
lastResponse.Let's rename it accordingly.
= nullisn't necessary since uninitialized reference variables are initialized withnullimplicitly.
-
I'd rename
s, newg, newf, newl to the clearer message, newGreeting, newFarewell, newLaugh.-
The naming in conjunction with the inverse logic of:
boolean notg = true;doesn't make your code clearer. I'd reverse the logic to:
boolean isGreeting = false;The same applies to
notf → isFarewell.-
Going through the class from the top raised the following questions:
// [...] >50% h's or l's → "What are h's and l's?"String ek → "What is an ek?"String k → "What is a k?" Only 8 lines later: Ah, it's a key. Let's name it accordingly.-
Though using the l-value of assignments is valid it doesn't make your code clearer:
System.out.println(response = ...);Sure, it gives you the opportunity to omit the braces for the preceding
if but doing that isn't recommended anyway.-
throw new ArithmeticException()prints just:
Exception in thread "main" java.lang.ArithmeticExceptionAn explaining message would be nice for the users of your program.
-
I'd rename
...Punc(), cap(), punc() to the clearer ...Punctuation(), capitalize(), punctuation().-
The
switch:switch((int) System.nanoTime() % 5) {
default:
case 0:
case 1:
case 2:
case 3:
return '.';
case 4:
return '!';
}can be simplified to:
switch((int) System.nanoTime() % 5) {
case 4:
return '!';
default:
return '.';
}-
If a read
//super hacky way of ... in a comment I'm highly alerted of seeing something weird.-
Genesis.java is rather long. I'd move all the static methods to a class GenesisUtil.FileManager
-
If I read a class name
FileManager I first think of the thing it is commonly used for: a File Manager.-
I'd separate logging and response file handling combined in
FileManager at the moment. - I'd also use one of the various Java logging frameworks instead of re-inventing the (logging) wheel.
-
Variable names like
resp, lout, rout, res aren't too descriptive. Is it responsibility? Is it loud misspelled? Is it route misspelled? Is it resolution?I'd call them
responseFileName, logWriter, responseWriter, responses.-
I'd rename
checkFiles() to createFiles() because that's what it does. Of course, only if one of them doesn't exist. But the result is the same in any case: The files exist after invocation of this method.-
You can shorten:
HashMap> res = new HashMap>();to:
HashMap> res = new HashMap<>();using the diamond operator.
-
Why are you using Unicode's 65533D as
split character?-
It's not recommended to omit braces in one-line blocks like:
for ( ... )
response = ...;-
I'd use:
String.format("[%s:%s:%s:%s]", hour, minute, second, ampm == 0 ? "AM" : "PM")instead of string concatenation:
"[" + hour + ":" + minute + ":" + second + ":" + (ampm == 0 ? "AM" : "PM") + "]"Such you also probably would have recognized easier that there is a
':' too much. And you could omit the add-zero-if-less-than-10 handling completely since the format string of String.format() can do this for you. -
I'd make variables with just two expected values like
ampm a boolean and call them isAM. If it is false it is the other.ResponseType
-
Why do you want to get an enum constant by a string
name derived from its field name? Such you're giving away the type safety enumerations give you by design. -
I'd implement the enum with a constructor:
public enum ResponseType {
GREETING("Greetings"),
FAREWELL("Farewell"),
VALUE("Value"),
LAUGH("Laugh");
String string;
ResponseType(String string) {
this.string = string;
}
@Override
public String toString() {
return string;
}
}Running Genesis
-
for the first time from within Eclipse Luna:
[02:10:53:PM]: Initializing Genesis v1.0...
[02:10:53:PM]: Generating files...
[02:10:53:PM]: Genesis v1.0 started on Windows Vista
You: hi
[02:10:57:PM]: Genesis v1.0 shut down with error code: 1
A fatal error occurred: java.lang.ArithmeticException: / by zero
...
This doesn't look like a problem relating to Genesis. Check the above general stack trace.
...I have my doubts about the second to last sentence. It is related to Genesis. Always! Since it happened when running it. Your end user doe
Code Snippets
boolean notg = true;boolean isGreeting = false;System.out.println(response = ...);Exception in thread "main" java.lang.ArithmeticExceptionswitch((int) System.nanoTime() % 5) {
default:
case 0:
case 1:
case 2:
case 3:
return '.';
case 4:
return '!';
}Context
StackExchange Code Review Q#97898, answer score: 13
Revisions (0)
No revisions yet.