HiveBrain v1.2.0
Get Started
← Back to all entries
principlejavaMinor

Little part of the string to integer parser vs Clean Code

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
theparserpartlittlecodestringintegerclean

Problem

I started reading a Clean Code book and some other articles on the Internet about OOD and design patterns and I'm a little confused. I've started to code a little string to integer parser which currently parses a first integer in string larger than 0 or returns -1.

Could you look and give me some idea if this is written correctly with general rules of good programming? I'm not sure about usage of Index instances in the IntegerSubstringer class.

main method

public static void main (String Args[])
    {
        IntegerParser parser = new IntegerParser();
        System.out.println( parser.getFirstInt("Blaola") );
    }


IntegerParser.java

package stringParser.integerParser;

public class IntegerParser
{
    IntegerSubstringer substringer;

    public IntegerParser()
    {
        substringer = new IntegerSubstringer();
    }

    public int getFirstPositiveInt(String textToParse)
    {
        String numberSubstring = substringer.cutFirstNumber(textToParse);
        if(!numberSubstring.equals(""))
            return parse(numberSubstring);
        return -1;
    }

    private int parse(String substringContainingNumbersOnly)
    {
        return Integer.parseInt(substringContainingNumbersOnly);
    }
}


IntegerSubstringer.java

```
package stringParser.integerParser;

public class IntegerSubstringer
{
IntegerFinder finder;
String text;
Index startIndex;
Index endIndex;

public IntegerSubstringer()
{
finder = new IntegerFinder();
}

public String cutFirstNumber( String text )
{
this.text = text;
setStartIndexAtTheFirstNumber();
iterateToTheEndOfTheNumberSeries();
if (startIndex.isInvalid()) return new String();
return makeSubstringAccordToIndexes();
}

private void iterateToTheEndOfTheNumberSeries()
{
setEndIndexAfterStartIndex();
while( endOfTextNotReached() && finder.letterIsANumber(text, endIndex) )
{
endIndex.

Solution

Index = int

My first thought as I skimmed through your code was that you have a whole class that seems to be nothing more than a wrapper for an int. The only method of any significance is isInvalid() which could easily be moved to somewhere else or just inlined when needed. I think it would be best to remove this class and just use ints as indices.
KISS

I didn't read the rest of the code very thoroughly but it seems like a lot of code to solve a simple problem. I prefer the KISS methodology (keep it simple stupid) to complicated solutions. I'm sure if you tried to rewrite your code with simplicity as a goal, you could come up with something much smaller and simpler.

Here's an example of how you could simplify. You have this function:

private int parse(String substringContainingNumbersOnly)
{
    return Integer.parseInt(substringContainingNumbersOnly);
}


Notice that all it does is call another function. You could remove this function and replace the call to it with the call to Integer.parseInt directly. Now someone reading the code will know exactly what your code is doing when they read that line. With your current code, they would have to skip to the definition of parse to figure out what is happening, because the function name "parse" doesn't really explain what it does.

In fact many of your functions are one liners. Instead of having these one line functions which are mostly called from one place only, you could just put the one line in the calling function and add a comment about what is happening (if it isn't clear already). Right now, it almost seems like every line of code is a function call to a new function.

Code Snippets

private int parse(String substringContainingNumbersOnly)
{
    return Integer.parseInt(substringContainingNumbersOnly);
}

Context

StackExchange Code Review Q#88732, answer score: 4

Revisions (0)

No revisions yet.