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.

I define a Pixel data with different size like this.

data Pixel = RGB8 Word8 Word8 Word8
           | RGBA8 Word8 Word8 Word8 Word8
           | RGB16 Word16 Word16 Word16
           | RGBA16 Word16 Word16 Word16 Word16
           | Empty
           deriving (Eq)

and Num instance

instance Num Pixel where
  x + Empty = x
  Empty + x = x
  (RGB8 ru gu bu) + (RGB8 rv gv bv) = RGB8 (ru + rv) (gu + gv) (bu + bv)
  (RGBA8 ru gu bu au) + (RGBA8 rv gv bv av) = RGBA8 (ru + rv) (gu + gv) (bu + bv) (au + av)
  (RGB16 ru gu bu) + (RGB16 rv gv bv) = RGB16 (ru + rv) (gu + gv) (bu + bv)
  (RGBA16 ru gu bu au) + (RGBA16 rv gv bv av) = RGBA16 (ru + rv) (gu + gv) (bu + bv) (au + av)
  x - Empty = x
  Empty - x = x
  (RGB8 ru gu bu) - (RGB8 rv gv bv) = RGB8 (ru - rv) (gu - gv) (bu - bv)
  (RGB16 ru gu bu) - (RGB16 rv gv bv) = RGB16 (ru - rv) (gu - gv) (bu - bv)
  (RGBA8 ru gu bu au) - (RGBA8 rv gv bv av) = RGBA8 (ru - rv) (gu - gv) (bu - bv) (au - av)
  (RGBA16 ru gu bu au) - (RGBA16 rv gv bv av) = RGBA16 (ru - rv) (gu - gv) (bu - bv) (au - av)
  _ * _ = error "Not support multiply"
  abs  = id
  signum _ = error "Not support signum"
  fromInteger _ = error "Not support fromInteger"

It's very ugly and bored. I think Haskell is a Elegant language, so can I have a better style to write this?

share|improve this question

2 Answers 2

up vote 4 down vote accepted

Above all, such an instance doesn't make any sense. A composite entity can't have much in common with a number, which the fact that your instance consists of just partial and undefined functions is only the evidence of.


With regards to your concern about the verbosity of the instance declaration, it was caused by you choosing an incorrect approach to type declaration.

Pixels of different bit-depths are appropriately described as different types, and not different constructors of the same type. The analogy is right before you: Word8 and Word16 are different types, not constructors.

Taking the said above into account I recommend you to reapproach your problem the following way:

-- The separate datatypes. Note the absense of `Empty` constructors, as there
-- is no such thing as empty pixel. There are black and transparent pixels,
-- which should be described as `PixelRGB8 0 0 0` or `PixelRGBA8 0 0 0 0`,
-- but not empty. And for dealing with missing pixels we have a `Maybe` type.
data PixelRGB8 = PixelRGB8 Word8 Word8 Word8
-- ...
data PixelRGBA16 = PixelRGBA16 Word16 Word16 Word16 Word16

-- We're taking an analogous approach to Num/Integral typeclasses, while 
-- combining both the "to" and "from" functionality in a single typeclass.
class Pixel a where
  -- whether its toPixelRGBA16 or possibly toRGBA64Pixel should depend on
  -- the highest precision type you want to be dealing with
  toPixelRGBA16 :: a -> PixelRGBA16
  fromPixelRGBA16 :: PixelRGBA16 -> a
  -- And here's addition for pixels of same type:
  addPixel :: a -> a -> a

instance Pixel PixelRGB8 where
  toPixelRGBA16 = error "TODO"
  fromPixelRGBA16 = error "TODO"
  addPixel = error "TODO"

instance Pixel PixelRGBA16 where
  toPixelRGBA16 = id
  fromPixelRGBA16 = id
  addPixel = error "TODO"

-- Here's addition for pixels of differing types:
addArbitraryPixel :: (Pixel a, Pixel b) => a -> b -> PixelRGBA16
addArbitraryPixel a b = addPixel (toPixelRGBA16 a) (toPixelRGBA16 b)
share|improve this answer
    
But I need pixel addition and pixel has different data type (8-bit, 16-bit). Do you have any idea to define a pixel? Thanks for answer –  Pikaurd Mar 6 '13 at 3:45
    
@Pikaurd Volkov's right. As for what you should do, check the Vector a type from the following link: learnyouahaskell.com/making-our-own-types-and-typeclasses The article is a good introduction to the subject. –  abuzittin gillifirca Mar 6 '13 at 6:56
    
@abuzittingillifirca Thanks your link. I'll try to rewrite it. –  Pikaurd Mar 6 '13 at 8:20
    
@Pikaurd See the updates to the answer –  Nikita Volkov Mar 6 '13 at 14:44
    
@NikitaVolkov Very enlightened Thanks. –  Pikaurd Mar 6 '13 at 15:39

I agree with the other answers saying that you should have different types for your different pixels.

That said, you can probably define a single polymorphic type and write a lot less code:

data Pixel a n = Pixel (a n) n n n

data NoAlpha n = NoAlpha

newtype Alpha n = Alpha n

type PixelRGB8 = Pixel NoAlpha Word8

type PixelRGBA8 = Pixel Alpha Word8

type PixelRGB16 = Pixel NoAlpha Word16

type PixelRGBA16 = Pixel Alpha Word16

Then, you can define pixel addition once for all the types (with a little helper for the alpha types):

class AddAlpha a where
  addAlpha :: (Num n) => a n -> a n -> a n

instance AddAlpha NoAlpha where
  addAlpha _ _ = NoAlpha

instance AddAlpha Alpha where
  addAlpha (Alpha a) (Alpha b) = Alpha (a + b)

addPixels :: (Num n, AddAlpha a) => Pixel a n -> Pixel a n -> Pixel a n
-- I'll leave the definition up to you.
share|improve this answer

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.