Problem
I’m trying to create an iterator, that will generate all the words that are one change away.
For example, if my input is “sam”, I should get the following words:
“aam”, “bam”, …”ram”,”tam”,….”zam”,”sbm”,”scm”,…”szm”,”saa”,”sab”,…”san”,”sao”,…”saz”.
Notice that there is no “sam” in the output, as it is 0 changes away.
The following function, it(), is a simple function that does this and prints the output:
public class oneChangeAway implements Iterator {
String s;
oneChangeAway(String s)
{
this.s =s;
}
@Override
public boolean hasNext() {
return false;
}
@Override
public Object next() {
return null;
}
@Override
public void remove() {
}
public void it() {
int len = s.length();
String temp ;
for (int i = 0; i < len; i++)
{
char[] char_s = s.toCharArray();
for(char c= 'a'; c<='z'; c++)
{
char_s[i] = c;
temp = new String(char_s);
if ( temp.equals(s)) continue;
System.out.println(temp);
}
}
}
}
However, now I want an iterator to do this. This is what I came up with:
public class oneChangeAway implements Iterator {
String s;
int l;
int i ;
char c ;
char[] c_s;
oneChangeAway(String s)
{
this.s =s;
l =s.length();
i = 0;
c = 'a';
c_s = s.toCharArray();
}
@Override
public boolean hasNext() {
if ( i == l)
return false;
return true;
}
@Override
public Object next() {
String temp;
if ( this.hasNext())
{
c_s[i] = c;
c++;
temp = new String(c_s);
if( c == ('z' + 1))
{
c = 'a';
c_s[i] = s.toCharArray()[i];
i++;
}
if (temp.equals(s)) return this.next();
return temp;
}
return null;
}
@Override
public void remove() {
}
public void it() {
int len = s.length();
String temp ;
for (int i = 0; i < len; i++)
{
char[] char_s = s.toCharArray();
for(char c= 'a'; c<='z'; c++)
{
char_s[i] = c;
temp = new String(char_s);
if ( temp.equals(s)) continue;
System.out.println(temp);
}
}
}
}
The following main function calls it:
public class App {
public static void main(String args[])
{
oneChangeAway ia = new oneChangeAway("sam");
//ia.it();
while (ia.hasNext())
{
System.out.println(ia.next());
}
}
}
This seems to work and the output is as intended. However, am I doing it the ‘right’ way?
Solution
am I doing it the ‘right’ way?
Please excuse the rude answer but: No.
general critic
Java is a class based object oriented language. So you should solve the problem in an object oriented way. This does not nessesarrily mean that your approach needs more classes and objects, but it does mean that you should apply the single responsibility pattern and find better separation of concerns.
Eg.: The responsibility of an Iterator is to give access to individual elements of a collection or stream like data structure. But the creation of this elements is not.
detailed critic
formal issues
naming
The way you choose the names in you approach has some problems.
First of all you should stick to the Java Naming Conventions In particular class names should always start with an upper case letter and the first part in a method name should be a verb.
Your variables should have readable names taken from the problem domain. You have one letter abreviations. It is quite certain that you don’t remember the meaning of that one letter variables yourself when you read this in 2 month…
magic numbers
In your approach you use magic numbers, even if they are chars actually. You should rather use constants with proper names like LETTER_WITH_LOWEST_ASCII_VALUE instead of 'a'.
returning literal null explicitly
You should never return a literal null reference explicitly unless it is a valid part of the result space.
Especially you should not indicate an error by returning null. Use exceptions instead. In your case the next() method should throw NoSuchElementException instead of returning null.
single line blocks
If you omit the curly braces for single line blocks then at least proper indent this line.
explicit use of literal true/false
In your hasNext() method you explicitly return the literal true/false values. This introduces the possibility to change the methods logic by accident. Why didn’t you simply return the result of the compare instead of wrapping it into an if statement?
use of continue
continue and break are the poor mans goto. In 20+ years of coding in Java I never ever used a continue or a break (other than inside a switch where break is inevitable).
So instead of placing a continue think of how you could write your code without it, which is very simple in your case.
raw types
The Iterator interface supports a generics parameter but your implementation does not use it. Therefore your next() method is declared to return a Object and the caller of your code must explicitly cast the value to a String relying on you that you don’t return something else.
broken OO principles
separation of concerns / single responsibility
As already mentioned the creation of the result collection is out of scope of an Iterators responsibility.
Therefore you should have a separate class or a method in your “main app” that creates some Collection or a Java8-Stream containing the result elements. Then there would be no need to write your own Iterator implementation at all.
other missed best practices
single abstraction layer
single abstraction layer means that a method either calls other methods or does some “low level” operations, but not both. (only loops, and conditionals are allowed in both method types).
You method next() does both: it calls hasNext() and the does some operations to create the next element. The code inside the if block which does the “low level operations” should be moved to a separate method so that hasNext() only calls other methods.