up vote 10 down vote favorite
2

I solved exercise 8 from Core Java for Impatient, chapter 1. This exercise is to implement a displayAllCombinations method to display all substrings of the given string.

I would like to prove that following algorithm is valid in Java. I wonder if there is an easier solution? 

package pl.hubot.exercises.corejava.r01.ex08;

import java.util.Scanner;

/**
 * This program contains mine solutions of exercises
 * from book Core Java for the Impatient, chapter 1.
 *
 * The content of following exercise:
 * "Write a program that reads a string and displays all contained
 * in it non-empty strings."
 *
 * @version 1.00 23-07-2016
 * @author Hubot
 */
public class Program
{
    public static void main(String[] args)
    {
        System.out.print("Enter any string: ");
        Scanner in = new Scanner(System.in);
        String text = in.nextLine();
        displayAllCombinations(text);
    }

    private static void displayAllCombinations(String text)
    {
        int offset = 0;
        int current = 0;
        int ending = text.length();
        while (offset != ending)
        {
            String result = text.substring(current, ending - offset);
            if (!result.equals(""))
                System.out.println(result);
            if (current != (ending - offset)) current++;
            else
            {
                current = 0;
                offset++;
            }
        }
    }
}

Sample run:

Enter any string: Amy
Amy
my
y
Am
m
A
share|improve this question
up vote 4 down vote accepted

The variable names offset, current, and ending could be better. offset seems to be distance from the end, and current acts as a starting index, so the names should reflect that.

The loop logic is cryptic. There is a current = 0; offset++; that "resets" the loop, so it would be more clearly written with nested for loops instead.

If you do it right, you shouldn't need to check !result.equals(""). If you did need to check, then it would be better written as !result.isEmpty().

private static void displayAllCombinations(String text) {
    for (int end = text.length(); end > 0; end--) {
        for (int start = 0; start < end; start++) {
            System.out.println(text.substring(start, end));
        }
    }
}
share|improve this answer
up vote 3 down vote 
        if (!result.equals(""))
            System.out.println(result);
        if (current != (ending - offset)) current++;
        else
        {
            current = 0;
            offset++;
        }

This is hard to read. On one occasion you've opted for no braces, same line, on another occasion you opted for no braces, multi-line, and later again you go for braces, multi-line.

Be consistent with the placement of your braces. It makes it easier to read the code.

    while (offset != ending)
    {
        String result = text.substring(current, ending - offset);
        if (!result.equals("")) 
        {
            System.out.println(result);
        }
        if (current != (ending - offset)) 
        {
            current++;
        }
        else
        {
            current = 0;
            offset++;
        }
    }

Beyond that, this program seems trivial and there's not much else to say about it.

There are, of course, various minor improvements we could make, such as...

  • Storing ending - offset in a variable called endIndex and calling current beginIndex to match substring's argument names

  • Rewriting the handling of beginIndex (current): 

    beginIndex++;
if (beginIndex== endIndex) 
{
    beginIndex = 0;
    offset++;
}

    This gets rid of the empty string case, whilst also simplifying your if-else statement.

  • Then, since we no longer need to check if the substring is an empty string, we just print the substring directly.

Such improvements would leave you with a result like so:

private static void displayAllCombinations(String text)
{
    int offset = 0;
    int beginIndex = 0;
    int ending = text.length();
    while (offset != ending)
    {
        int endIndex = ending - offset;
        System.out.println(text.substring(beginIndex, endIndex));
        beginIndex++;
        if (beginIndex == endIndex) 
        {
            beginIndex = 0;
            offset++;
        }
    }
}

As for testing its correctness, I'd use unit tests, and instead of printing to screen, you add the strings to a List and return the list. Interesting cases are empty string, single character string, and a regular string like "Amy".

share|improve this answer
    
nitpick, in Java they use egyptian braces – Dan Pantry Jul 24 '16 at 11:12
    
@DanPantry I'm just copying his style – Pimgd Jul 24 '16 at 14:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.