5
\$\begingroup\$

I came across this problem.

Problem Statement:

Given an array of ints, is it possible to choose a group of some of the ints, beginning at the start index, such that the group sums to the given target? However, with the additional constraint that all 6's must be chosen.

  • groupSum6(0, {5, 6, 2}, 8)true
  • groupSum6(0, {5, 6, 2}, 9)false
  • groupSum6(0, {5, 6, 2}, 7)false

Below is my code:

public boolean groupSum6(int start, int[] nums, int target) {
if(nums.length==0)
  return target==0;

   if(start==nums.length-1)
    return target==nums[nums.length-1]||(target==0&&!(nums[nums.length-1]==6));

    if(target==0&&nums[start]==6)
    return false;
    if(target==0&&!(nums[start]==6))
      return groupSum6(start+1, nums, 0);

      if(groupSum6(start+1, nums, target-nums[start])||nums[start]==6)
      return groupSum6(start+1, nums, target-nums[start]);

      return groupSum6(start+1, nums, target);
}

Please feel free to review, or if you have a better solution in mind which uses lesser number of ifs, you can share it in your review.

EDIT: The code was written using a plain text editor so it's less readable, for a readable version of code you may have a look here As there is already one review which explains about readability so request to review keeping the code optimization in view.

\$\endgroup\$
  • \$\begingroup\$ Is nobody interested in this one? lol \$\endgroup\$ – Anirudh Jul 12 '14 at 18:12
  • 1
    \$\begingroup\$ quick question, does the constraint "take all 6es" apply also to the elements before the starting index? \$\endgroup\$ – Vogel612 Jul 17 '14 at 11:42
  • \$\begingroup\$ What is the purpoe of that first argument? \$\endgroup\$ – Jeroen Vannevel Jul 17 '14 at 11:43
  • 1
    \$\begingroup\$ @JeroenVannevel it's the starting index... i think# \$\endgroup\$ – Vogel612 Jul 17 '14 at 11:44
  • \$\begingroup\$ @Vogel612 all the 6's infact all the members of the array are to be scanned so you would want to pass the first argument as 0. \$\endgroup\$ – Anirudh Jul 17 '14 at 11:48
3
\$\begingroup\$

This will be a solely style-focused review, because your code (don't take it personal) looks horrible...

SPACE to breathe:

Your code is cramped, and your poor operators and statments a sqeezed together like sardines in a tin.

Spaces are free! Use them!

Tabs to clarify:

You seem to have an aversion to indenting your code, or rather really indenting it. You indent by 2 spaces. That's a little thrifty. Use one tab or four spaces instead. This makes it easier to distinguish between levels, especially when you have large blocks of indented code in one method.

Braces to see:

Additionally you seem to dislike Braces to separate blocks. So here goes:
Wherever optional, still use braces!

Why? Because it's clear, consistent and less prone to bugs when you do.

Ctrl+Shift+F to format:

Use your IDE!!! Eclipse and Netbeans are both free, and both can do much of the formatting for you.

Your Code, after Applying just these three things:

public boolean groupSum6(int start, int[] nums, int target) {
    if (nums.length == 0) {
        return target == 0;
    }

    if (start == nums.length - 1) {
        return target == nums[nums.length - 1] || 
                (target == 0 && !(nums[nums.length - 1] == 6));
    }    

    if (target == 0 && nums[start] == 6) {
        return false;
    }
    if (target == 0 && !(nums[start] == 6)) {
        return groupSum6(start + 1, nums, 0);
    }

    if (groupSum6(start + 1, nums, target - nums[start]) || nums[start] == 6) {
        return groupSum6(start + 1, nums, target - nums[start]);
    }

    return groupSum6(start + 1, nums, target);
}

I think this is much more readable

\$\endgroup\$
  • 2
    \$\begingroup\$ You took styling to another level. Funny review. :-] \$\endgroup\$ – Anirudh Jul 17 '14 at 13:01
  • \$\begingroup\$ I do know it's important to take care of spaces and tabs it, I agree it would have been a minute's effort to copy paste this code on the IDE and have the indentation sorted. I do not think curly braces are necessary for if statements that do not include blocks..look up collabedit.com/36jms if readability is what one desires that this is enough for me. \$\endgroup\$ – Anirudh Jul 17 '14 at 13:10
  • 1
    \$\begingroup\$ @Anirudh: It's not just about readability. If you have to come back and add an extra line, you will have to add in the braces. If you forget to do that, one of the lines will execute unconditionally. You probably won't forget if you need to change the algorithm, but if you just need to add a log statement for debugging the code, you might not think of it. \$\endgroup\$ – unholysampler Jul 17 '14 at 13:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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