3
\$\begingroup\$

It's an exercise from a Swift-course I'm currently taking.

You are given an array of integers. Make an additional array named computedNumbers. computedNumbers should multiply each element in the numbers array by the next following element. The last element shall be multiplied by the first element.

Example: If numbers was equal to ... [3, 1, 4, 2] ... then the computedNumbers should computed [3 x 1, 1 x 4, 4 x 2, 2 x 3], so that they become: [3, 4, 8, 6]

Here my solution:

var numbers = [45, 73, 195, 53]

var computedNumbers = [Int]()
let count = numbers.count
var i = 0

while i < count - 1 {
    computedNumbers.append(numbers[i] * numbers[i + 1])
    i = i + 1
}

computedNumbers.append(numbers[count - 1] * numbers[0])

print(computedNumbers)

It has passed the automated tests, but it isn't very elegant.

It there a better way to accomplish the task?

What should I have done different and why?

Looking forward to reading your comments and answers.

\$\endgroup\$
  • 2
    \$\begingroup\$ A possible alternative one-liner: numbers.indices.map { numbers[$0] * numbers[($0 + 1) % numbers.count] } \$\endgroup\$ – ielyamani Nov 18 '19 at 13:51
6
\$\begingroup\$

Handle edge cases

If the given numbers array is empty then the first loop will do nothing, but

computedNumbers.append(numbers[count - 1] * numbers[0])

aborts with a runtime exception. The correct result in this case would be to set computedNumbers to an empty array as well.

Better loops

This

var i = 0
while i < count - 1 {
    // ...
    i = i + 1
}

is shorter and better done as

for i in 0 ..< count - 1 {
    // ...
}

The advantages are

  • The scope of i is limited to the loop.
  • i is a constant.
  • The range of i is clearly seen right at the start of the loop.

In this particular case we iterate of adjacent array elements, which is conveniently by by zip()ping the array with a shifted view of itself:

var computedNumbers = zip(numbers, numbers.dropFirst()).map(*)
computedNumbers.append(numbers[count - 1] * numbers[0])

If we append the first element to the shifted view then the complete operation reduces to

let computedNumbers = zip(numbers, [numbers.dropFirst(), numbers.prefix(1)].joined()).map(*)

Note that this handles also the case of an empty array gracefully.

Use constants if possible

The numbers array is never mutated, therefore it should be declared as a constant with let:

let numbers = [3, 1, 4, 2]

Using a constant

  • makes it clear to the reader that this value is never mutated,
  • helps to avoid unintentional mutation,
  • possibly allows better compiler optimization.

Make it a function

If we put the functionality in a separate function then

  • it becomes reusable,
  • test cases can be added more easily,
  • we can give it a meaningful name,
  • we can add documentation.

In our case the function would simply be

/// Compute an array where each element in the numbers array is multipled by
/// the next following element. The last element is multiplied by the first element.
/// - Parameter numbers: An array of integers.
func circularAdjacentProducts(of numbers: [Int]) -> [Int] {
    return zip(numbers, [numbers.dropFirst(), numbers.prefix(1)].joined()).map(*)
}

and would then be used as

let numbers = [3, 1, 4, 2]
let computedNumbers = circularAdjacentProducts(of: numbers)
print(computedNumbers)

Make it generic on the element type

The same operation could be done with an array of floating point values (Float or Double), or with other integer types (e.g. Int16) as long as the multiplication does not overflow. Therefore we can generalize it to array of type [T] where T conforms to a protocol which required a multiplication operation.

A possible choice is the Numeric protocol which is adopted by all floating point and integer types:

func circularAdjacentProducts<T: Numeric>(of numbers: [T]) -> [T] {
    return zip(numbers, [numbers.dropFirst(), numbers.prefix(1)].joined()).map(*)
}

Example:

let floats = [3.3, 1.1, 4, 2.2]
let computedFloats = circularAdjacentProducts(of: floats)
print(computedFloats) // [3.63, 4.4, 8.8, 7.26]

Make it generic on the collection type

The next possible generalization is to apply this operation not only to arrays, but to arbitrary collections:

extension Collection where Element: Numeric {
    func circularAdjacentProducts() -> [Element] {
        return zip(self, [dropFirst(), prefix(1)].joined()).map(*)
    }
}

Example with an ArraySlice:

let numbers = Array(1...10).dropFirst(3)
print(numbers) // [4, 5, 6, 7, 8, 9, 10]
let computedFloats = numbers.circularAdjacentProducts()
print(computedFloats) // [20, 30, 42, 56, 72, 90, 40]
| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

If I remember correctly, at the time that exercise had been given, using loops had not been covered, so you did very well to discover the while loop and how to use it.

The point of that exercise was to show you how tedious it is to write essentially the same code for each different element:

let numbers = [3, 1, 4, 2]
var results = [Int]()

results.append(numbers[0] * numbers[1])
results.append(numbers[1] * numbers[2])
results.append(numbers[2] * numbers[3])
results.append(numbers[3] * numbers[0])

// and even maybe
print(results[0])
print(results[1])
print(results[2])
print(results[3])

// to print individually

...and that introduces the reason for using loops.

So your solution already went way beyond what was asked, so give yourself a pat on the back for that.

Martin's answer gives excellent advice on loops and those will also be covered in the next step of the course, but don't concern yourself too much about the more advanced stuff (like generics) for now. They are an important part of what makes Swift so expressive and so enjoyable to use, but they are an advanced topic and come later, once the basics have been addressed.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! This nicely explains what not to do but the question is asking what to do instead. Can you give advice on how to improve the code in the question? \$\endgroup\$ – Null Nov 21 '19 at 16:36
  • \$\begingroup\$ I think the part 'Better Loops' in Martin's answer is sufficient for that. My answer was really only because I recognise the exercise from an online course, where you can submit your answers and I assume Michale may want to submit his answer and is wanting to improve it, which Martin's answer addresses. I just wanted to assure him that loops are not an expected part of that answer because the course hasn't yet introduced them. This exercise expects the answer in long form, without loops, as I showed, but the loop form passes too. If it's not an appropriate answer for here, I'm happy to delete. \$\endgroup\$ – Najinsky Nov 21 '19 at 17:07

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.