Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

So I would be very grateful if you could check these four tasks - I'm a Java newbie and recently coded such things on a Introduction to CS test which I didn't score so well at and would love to know where my mistakes were, what I should do some other way and how can I get better at it to score higher again :)

1> Write a method that takes an array of integers (you can assume the amount is divisble by four) and decodes them in the said manner:

  • add 15 to each integer unless it's equal to 0
  • divide the numbers in four-integer sub-words
  • let the first charater become the third, the fourth one be the first, the third - the second and the second be the last

    For example, {97,90,82,0} should output " api".
public static String decipher(int[] specArray)
{
    String finalWord = "";

    for(int i=0; i<specArray.length; ++i)
    {
        if(specArray[i]!=0) specArray[i]+=15;
    }

    for(int i=0; i<specArray.length; i+=4)
    {
        finalWord+=(char)specArray[i+3];
        finalWord+=(char)specArray[i+2];
        finalWord+=(char)specArray[i];
        finalWord+=(char)specArray[i+1];
    }

    return finalWord;
}

2> Write a method that takes a String as an argument and also returns a String and processes it from left to right as follows:

  • if it finds a, it should add two a's to the result
  • if it finds two adjacent b's or one b alone, it should add one b to the result
  • if it finds two c's, it should add them both to the result. If it finds one c only, it should skip it.


For example: "abaabbcbaccb" returns "aabaaaabbaaccb" and "bbbbbbb" returns "bbbb".

public static String processWord(String inputWord)
{
    String outputWord = "";

    for(int i=0; i<inputWord.length(); ++i)
    {
        switch(inputWord.charAt(i)){
            case 'a':
                outputWord+="aa";
                break;

            case 'b':
                if(((i+1)<inputWord.length())&&(inputWord.charAt(i+1)=='b'))      // if there's at least one char left in the string and the next is b
                        {
                            i+=1;
                        }
                outputWord += 'b';
                break;

            case 'c':
                if(((i+1)<inputWord.length())&&(inputWord.charAt(i+1)=='c'))      // if there's at least one char left in the string and the next is c
                        {
                            outputWord += "cc";
                            i+=1;
                        }
                break;                

        }  
    }
    return outputWord;
}

3> Create a class MyClass with two class variables - myNumber of type double and myString of type String. Also, add a default constructor giving myNumber a value of 13.13 and myString value "some string" and a constructor taking a double and String parameters, then assigning them to myNumber and myString respectively. Add appropriate methods for getting and setting the values of the class.

public class MyClass {
private double myNumber;
private String myString;

// CONSTRUCTORS
public MyClass(){
    myNumber = 13.13;
    myString = "some string";
        }

public MyClass(double d, String s){
    myNumber = d;
    myString = s;
        }

// METHODS
public double getNumber()
{
    return myNumber;
}

public void setNumber(double newN)
{
    myNumber = newN;
}

public String getString()
{
    return myString;
}

public void setString(String newS)
{
    myString = newS;
}
}

4> Create a method working similarly to String.indexOf() but taking as parameters a source String in which it should look and String containing the word it should look for in the source. It should return an integer array containing all the occurrences of said sub-word in a word denoted by indexes of the first letters.

For example, ("ThiDFUs iDFs a DF D DFU sample.", "DFU") should return an array containing 3 and 20.

public static int[] differentIndexOf(String source, String divisor)
{
    ArrayList<Integer> occurrenceFound = new ArrayList<Integer>();
    int suspectedStart=-1;

    for(int i=0; i<source.length(); ++i)                                    // check the whole word letter-by-letter
    {
        if(source.charAt(i)==divisor.charAt(0))                            // if one of the letters corresponds the first one of the separator:
        {
            suspectedStart = i;
            for(int j=1; j<divisor.length(); ++j)                          // check the ones after it until the separator is fully used
            {
                if(source.charAt(i+j) != divisor.charAt(j)) suspectedStart = -1;  // (^) or until you find a difference
            }
        }

        if(suspectedStart != -1) {occurrenceFound.add(suspectedStart);}     // if no differences found in (^) => occurrence found
        suspectedStart = -1;
    }

    int[] finalArray = new int[occurrenceFound.size()];

    for(int i=0; i<finalArray.length; ++i)                             
    {
        finalArray[i] = occurrenceFound.get(i);
    }

    return finalArray;
}
share|improve this question

2 Answers 2

up vote 2 down vote accepted

Hmmmm...

1> This looks mostly okay. Probably the biggest help for you would be to extract adding the offset (15) to another method. This should change your algorithm from O(2n) to O(n); that is, you only have to loop through the array once, instead of twice:

private static final int OFFSET = 15;

private static char offsetCharacter(final int character) {
    return (char) (character == 0 ? character : character + OFFSET);
}

Also, string concatenation is not really that efficient; you should probably use a StringBuilder, which will allow for the fastest writes (well, probably). And a little error checking never hurt, even in cases where you are allowed 'assumptions. Oh, and some of your names could use a little work.

private static final int WORD_LENGTH = 4;

public static String decipher(final int[] encodedCharacters) {

    if (encodedCharacters == null || encodedCharacters.length == 0 || encodedCharacters.length % WORD_LENGTH != 0) {
       throw new IllegalArgumentException("Requires input array sized as multiple of " + WORD_LENGTH);
    }

    final StringBuilder decoded = new StringBuilder(encodedCharacters.length);

    for(final int i = 0; i < encodedCharacters.length; i += WORD_LENGTH) {

        decoded.append(offsetCharacter(encodedCharacters[i + 3]));
        decoded.append(offsetCharacter(encodedCharacters[i + 2]));
        decoded.append(offsetCharacter(encodedCharacters[i + 0]));
        decoded.append(offsetCharacter(encodedCharacters[i + 1]));

    }

    return decoded.toString();
}

Unfortunately, I couldn't figure out a good way to deal with the swapping...


2> You're using String concatenation again, when you should be using a StringBuilder. Things are a little convoluted here, probably due to the requirements for b/c (actually, I'm pretty sure that was the point). Ideally, you'd be able to use Regex. Unfortunately, I'm not that great with it at the moment.
I'm assuming that all other characters are ignored (like you do). Oh, even if there isn't anything to actually process, it's usually best to put a 'default' case in switch statements, as a deliberate declaration of expectation. Somehow, it seems a little clumsy right now, but I haven't been able to come up with anything better...


3> Nothing much to go wrong with here. You avoided the easy trap of making myString or myNumber non-private. Some general stuff:

Usually, when you have constructors that set defaults, and one that requires all parameters, make the 'defaulting' ones use the other:

public MyClass() {
    this(13.13, "some string");
}

public MyClass(double d, String s) {
    myNumber = d;
    myString = s;
}

Also, getters/setters tend to have the entire name of the variable, instead of just part of it (so, getMyNumber()). Note that this will depend on local coding styles to an extent; actually here, I'd probably prefer your choice, given the awkward names chosen by your professor. Also, for setters, the name of the passed in parameter is often the same as the parameter it's setting, and the this reference is used to differentiate them:

public void setNumber(double myNumber) {
    this.myNumber = myNumber;
}

Oh, and the requirements didn't list them, but it's a good idea to put Javadoc on every exposed member (class, variable, and method). Getters and Setters are usually pretty boring, though.


4> The requirements are a little sparse on this one. I mean, it doesn't restrict me from using something like the Apache Commons Utils. Heck, it doesn't say you aren't allowed to use String.indexOf() inside your method - although that obviously isn't the intent of the problem. I'm going to assume that you're limited to the 'standard' JDK libraries. And that you aren't allowed to use Regex here.
Oh, never use end-of-line comments.

ArrayList<Integer> occurrenceFound = new ArrayList<Integer>();

Whenever possible, use the least specific (most generic) object possible:

List<Integer> occurrenceFound = new ArrayList<Integer>();

Oh, you don't actually need to check every character in the string:

for(int i = 0; i < source.length(); ++i) {
    ....
}

After all, if you find the starting character as the last character, it's going to be a given it's not going to match, right? What's the actual last character you need to check?

int[] finalArray = new int[occurrenceFound.size()];

Learn your libraries - the interface Collection defines a couple of methods that would probably be helpful.

Breaking things up a little (and just using arrays) makes it a bit cleaner. I'd really prefer some sort of 'Collections' based approach (ie something that List.subList() would work with), but that's probably too 'heavy'.

public static int[] differentIndexOf(String source, String divisor) {

    final char[] characters = source.toCharArray();
    final char[] search = divisor.toCharArray();

    final int[] occurrences = new int[characters.length - search.length + 1];
    int occurrenceCount = 0;

    for (int i = 0; i < characters.length - search.length; i++) {
        if (stringMatches(characters, i, search)) {
            occurrences[occurrenceCount] = i;
            occurrenceCount++;
        }
    }

    return Arrays.copyOf(occurrences, occurrenceCount);
}

private static boolean stringMatches(char[] characters, int index, char[] search) {
    for (int i = 0; i + index < characters.length && i < search.length; i++) {
        if (characters[i + index] != search[i]) {
            return false;
        }
    }

    return true;
}
share|improve this answer
1  
Your big-O claim is confused. Technically, O(n) = O(2n), there is no difference between the two classes. In terms of efficiency there is no reason to believe that combining the two loops will help at all. –  Winston Ewert Jan 17 '13 at 20:33
    
@WinstonEwert - You're right, when compared to other progressions, there is no difference. And the array access are likely to be fast regardless. Still, it's usually best to attempt to reduce multiple iterations to one. –  Clockwork-Muse Jan 17 '13 at 20:48
1  
Why is it best to attempt to reduce the multiple iterations? –  Winston Ewert Jan 17 '13 at 20:51
    
@WinstonEwert - Assuming that array access has a non-zero cost (and all operations will still otherwise be performed), the 'cost' of the function has been reduced by the length of the input. 2bn array access rather than 4bn sounds better to me. While not as applicable to this situation, there are some languages that provide built-in mechanisms for operating on an array in parallel - however, they really only work for a single iteration. With chained function steps (and preferably more than here), programs can really fly over data processing. –  Clockwork-Muse Jan 18 '13 at 16:45
1  
Ack! You're right about the + 1 bit. However, given current requirements, you can't use characters.length / search.length - if you have a string of repeating characters, and a search string of the same characters (but shorter), there will be a 'starting' occurrence for every character (until search.length before the end). I'd wanted to use the divided version too, originally. –  Clockwork-Muse Jan 18 '13 at 18:45

In (3), its bad practice to update the counter of the for loop inside the body of the four loop.

It's better to use a while() loop here, since each iteration of the loop may consume multiple tokens from the input string.

For (4), this may be not be in the spririt of the question, but there already is an indexOf(substring, startIndex) method in Java which would be much more appropriate here. Not sure if you are restricted from using that though.

share|improve this answer
    
Thank you a lot :) Yep, I was restricted from using indexOf, forgot to mention it as it was only verbally stated, not mentioned in the specs ;) Also, why is additional incrementing of the counter inside for loop bad whilst being acceptable for while loop? –  Straightfw Jan 18 '13 at 18:26
1  
Its mostly about clarity. If we see for (int i=0; i < 100; i++), most developers automatically assume it will get executed 100 times. If we see a while or a do loop, we expect the number of iterations to be variable. –  JohnnyO Jan 18 '13 at 18:42
    
I see. Thanks a lot :) –  Straightfw Jan 18 '13 at 18: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.