patternjavaMinor
Validating an arithmetic sequence
Viewed 0 times
sequencevalidatingarithmetic
Problem
I'm looking for criticism on the following method used to determine if a collection of
Integers forms an arithmetic sequence:public boolean isArithmetic(Integer[] nums)
{
Set result = new HashSet();
Collections.sort(Arrays.asList(nums));
for(int i = 0; i 1)
return false;
return true;
}Solution
The basic method you're using seems to me rather roundabout. If you're going to write a loop to walk through the collection anyway, why not just check for the correct criteria there? e.g., something like this:
This can save quite a bit of work, and (especially if the numbers don't form an arithmetic sequence) may save a fair amount of storage as well. More importantly, it makes the intent immediately obvious, which (it seems to me) that inserting all the differences into a hash table doesn't.
One other specific detail of your code bothers me:
Anything of the form
If you really want to use the approach you've taken, I think it becomes a lot more reasonable if you take your loop and wrap it up into a generic algorithm. I'd probably call it
That's simple and neat enough that as long as you're sure this is outside the critical path (i.e., the extra time and space used don't matter) it would at least be worth considering.
Integer diff = nums[1] - nums[0];
for (int i=2; i<nums.length; i++)
if (nums[i] - nums[i-1] != diff)
return false;
return true;This can save quite a bit of work, and (especially if the numbers don't form an arithmetic sequence) may save a fair amount of storage as well. More importantly, it makes the intent immediately obvious, which (it seems to me) that inserting all the differences into a hash table doesn't.
One other specific detail of your code bothers me:
if(result.size() > 1)
return false;
return true;Anything of the form
if (x) return true; else return false; can be expressed as return x;. In your case, the sense is reversed, so it can be return !x;, so you just want return result.size() == 1; (but, as noted above, I'd rather eliminate this entirely).If you really want to use the approach you've taken, I think it becomes a lot more reasonable if you take your loop and wrap it up into a generic algorithm. I'd probably call it
adjacentDifference. Then you end up with something on this general order:Set result = new HashSet();
Collections.sort(Arrays.asList(nums));
Algorithms.adjacentDifference(nums, result);
return result.size() == 1;That's simple and neat enough that as long as you're sure this is outside the critical path (i.e., the extra time and space used don't matter) it would at least be worth considering.
Code Snippets
Integer diff = nums[1] - nums[0];
for (int i=2; i<nums.length; i++)
if (nums[i] - nums[i-1] != diff)
return false;
return true;if(result.size() > 1)
return false;
return true;Set<Integer> result = new HashSet<Integer>();
Collections.sort(Arrays.asList(nums));
Algorithms.adjacentDifference(nums, result);
return result.size() == 1;Context
StackExchange Code Review Q#120987, answer score: 8
Revisions (0)
No revisions yet.