All RGB colors in one image

This is a program to solve: Images with all Colors from Code Golf:


Make images where each pixel is a unique color (no color is used twice
and no color is missing).

• Create the image purely algorithmically.

• Image must be 256×128 (or grid that can be screenshot and saved at 256×128)

• Use all 15-bit colors (15-bit colors are the 32768 colors that can be made by mixing 32 reds, 32 greens, and 32 blues, all in equidistant steps and equal ranges. Example: in 24 bits images (8 bit per channel), the range per channel is 0..255 (or 0..224), so divide it up into 32 equally spaced shades.)

This is my first Scala application, and my first Object Oriented application for a long time.

So, I have a Picture class where you can manipulate(get/set) pixels; and an abstract Painter class which produces a (maybe partial) Picture. And there are different painters inheriting from it for different styles.

Please focus on the application design instead of the algorithm for drawing picture when criticising. I specifically need criticisms for:

-
OOP design: Did I use any anti-patterns? How would you model the solution?

-
Scala: Is this idiomatic Scala? What is missing?

• Syntax suggestions: It looks like Scala has many syntactic structures and sugars. Is there something which can shorten the code, or make it clearer?

Full code on GitHub, for additional context

```
import com.sksamuel.scrimage._
import java.awt.image.BufferedImage
import java.awt.image.BufferedImage._
import scala.collection.immutable.Seq
import scala.collection.mutable.Set
import scala.util._

object Main extends App {
val im1 = new DirectionalPainter( new Picture(256, 128)
, Set(LeftOblique, RightOblique)
).paint(0.1)
val im2 = new AvgPainter(im1).paint

im2.save("/tmp/testOut.png")
}

object Painter {
def colorSSD(c1: RGBColor, c2: RGBColor): Double = {
val RGBColor(r1, g1, b1, _) = c1
val RGBColo

For a first try in Scala, this code is a nice attempt.
There are not so many rudiments of imperative style programming, which are usually frequent.
It is also quite concise and structured, which produces a good overall impression.

However, there are of course a few things to adjust and to improve. I'll divide them in two categories:

-
language issues (programming constructs to use, refactorings or style remarks)

-
design issues (related with the structure of the application)

Language Issues

DRY It

The three functions of object Painter contain boilerplate repetitions of instructions like Math.pow(n1 - n2, 2) or (r / num).round.toInt.
Dedicated shortcut functions can be created in order to reduce number of arguments and chained calls, for example:

// as private function under Painter
private def squareDiff(v1: Int, v2: Int) = Math.pow(v1 - v2, 2)

// as nested function inside colorAvg
def colorAvgComp(compSum: Double) = (compSum / colors.size).round.toInt

/: vs foldLeft

The /: call in the else block of Painter.colorAvg look really heavy, especially with a single-case matcher.
As per Scaladoc, z /: xs is the same as xs foldLeft z. In general, foldLeft is used hugely and easier to read:

// we summarize the RGB components of colors:
val sumColors = colors.foldLeft((0.0, 0.0, 0.0)) {
  case ((accR, accG, accB), color) => (accR + color.red, 
                                       accG + color.green, 
                                       accB + color.blue)
  }

// and return the RGBColor Option using the colorAvgComp shortcut above:
Some(RGBColor(colorAvgComp(sumColors._1), 
              colorAvgComp(sumColors._2), 
              colorAvgComp(sumColors._3)))

In the if condition of the same method, isEmpty is usually a better choice to check if a collection contains something, than length == 0.

for Loops & Comprehensions Consistency

There are weird differences between for loop definitions that are supposed to do similar things. Just be consistent,
use either Stream.range or until, but not both. Semicolons are very bizarre at line start and indeed are not necessary.

The for - yeild comprehension produces a collection that can be directly converted using toSet, no need for additional varargs :_* conversions:

// in Painter.possibleColors:
(for { r <- Stream.range(0, depth)
       g <- Stream.range(0, depth)
       b <- Stream.range(0, depth)
    } yield RGBColor(r * mul, g * mul, b * mul)).toSet

Avoid Main Logic in Assertions

assert occurs several times and this is something to appreciate. But the general principle is that assertions are used
to validate pre/post-conditions or states, especially in dev phase and can be toggled on/off when necessary. They must not contain
calls that change the state or pieces of main logic. If assertions are switched off, the main logic instructions passed
to assert will just not be executed and the program will not produce the expected results.

With assertions, the following approach should be used:

val result = myObj.ensureJobExecuted(arg1, arg2)
assert(result)

So in the original code:

// this assertion is OK (but !directions.isEmpty would be more usual)
assert(directions.size > 0)

// this assertion is bad: it changes the state!
assert(set(coord, color)) // FIXME: extract the main logic call outside assert!

Boolean Expressions

These ones occur in the code:

def coordValid(coord: Coord): Boolean = 0 <= coord.x && coord.x < width && 0 <= coord.y && coord.y < height

assert(0 <= percentage && percentage <= 1)

When checking them a bit deeper, I saw that this approach just reproduces some math expressions like 0

-
changes the possible coordinates collection

-
outputs the percentage value

-
sets the pixel color value (this is what it is intended to do)

Clearly, it is not very friendly with the SRP!

Another question is why does it check if the pixel is already filled and if the color is available? I think that this
validation should be produced outside.

Colors and Coordinates

The dedicated vals for possible colors and coordinates look heavy, especially in the context when they are iterated,
mapped or sorted on each call in different implementations of step function.

What improvements I can suggest:

-
Painter class will declare colors and coordinates as abstract members, typed as Seq of respective type. The implementors will
need to initialize these sequences with the order of items required by the implementor. This will allow to iterate on these collections
directly on the abstract layer and make them immutable. For example, LinearPainter will fill the Seqs from min to max; RandomPainter will
fill them randomly, etc.

-
Painter.paint function will acquire iterators on colors and coordinates sequences and iterate on their elements
until the required percentage is reached.

-
step` (named properly) will have a default implementa

StackExchange Code Review Q#145911, answer score: 7