Iterator to generate all the words (of a given words) that are one change away

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

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.

StackExchange Code Review Q#157016, answer score: 4