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.

Essentially I'm wondering if this implementation of an interface is correct. I mostly don't like having to repeat the exact same getters and setters. Is there a better way or is this alright?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Microsoft.Xna.Framework;

namespace TetrisPro
{
    interface Block
    {
        Color GetColor();
        Position[] GetPosition();
        Position GetPosition(int i);
        Position[] GetOldPosition();
        Position GetOldPosition(int i);
    }
    class Square : Block
    {
        private Color color;
        private Position[] position;
        private Position[] oldPosition;

        public Square()
        {
            this.color = Color.Firebrick;
            this.position = new Position[4];
            this.oldPosition = new Position[4];
            SetPosition();
        }
        public Square(int x, int y)
        {
            this.color = Color.GreenYellow;
            this.position = new Position[4];
            this.oldPosition = new Position[4];
            SetPosition();
            SetPosition(x, y);
        }

        public void SetPosition()
        {
            this.position[0] = new Position(0, 0);
            this.position[1] = new Position(0, 1);
            this.position[2] = new Position(1, 0);
            this.position[3] = new Position(1, 1);
        }
        public void SetPosition(int x, int y)
        {
            this.oldPosition = position;
            this.position[0] = new Position(x, y);
            this.position[1] = new Position(x, y + 1);
            this.position[2] = new Position(x + 1, y);
            this.position[3] = new Position(x + 1, y + 1);
        }
        public Color GetColor()
        {
            return this.color;
        }
        public Position[] GetPosition()
        {
            return this.position;
        }
        public Position GetPosition(int i)
        {
            return this.position[i];
        }
        public Position[] GetOldPosition()
        {
            return this.oldPosition;
        }
        public Position GetOldPosition(int i)
        {
            return this.oldPosition[i];
        }
    }
    class Line : Block
    {
        private Color color;
        private Position[] position;
        private Position[] oldPosition;

        public Line()
        {
            this.color = Color.BlueViolet;
            this.position = new Position[4];
            this.oldPosition = new Position[4];
            SetPosition();
        }

        public void SetPosition()
        {
            this.position[0] = new Position(0, 0);
            this.position[1] = new Position(0, 1);
            this.position[2] = new Position(0, 2);
            this.position[3] = new Position(0, 3);
        }
        public void SetPosition(int x, int y)
        {
            this.oldPosition = position;
            this.position[0] = new Position(x, y);
            this.position[1] = new Position(x, y + 1);
            this.position[2] = new Position(x, y + 2);
            this.position[3] = new Position(x, y + 3);
        }
        public Color GetColor()
        {
            return this.color;
        }
        public Position[] GetPosition()
        {
            return this.position;
        }
        public Position GetPosition(int i)
        {
            return this.position[i];
        }
        public Position[] GetOldPosition()
        {
            return this.oldPosition;
        }
        public Position GetOldPosition(int i)
        {
            return this.oldPosition[i];
        }
    }
    class ZLeft : Block
    {
        private Color color;
        private Position[] position;
        private Position[] oldPosition;

        public ZLeft()
        {
            this.color = Color.DodgerBlue;
            this.position = new Position[4];
            this.oldPosition = new Position[4];
            SetPosition();
        }

        public void SetPosition()
        {
            this.position[0] = new Position(0, 0);
            this.position[1] = new Position(1, 0);
            this.position[2] = new Position(1, 1);
            this.position[3] = new Position(2, 1);
        }
        public void SetPosition(int x, int y)
        {
            this.oldPosition = position;
            this.position[0] = new Position(x, y);
            this.position[1] = new Position(x + 1, y);
            this.position[2] = new Position(x + 1, y + 1);
            this.position[3] = new Position(x + 2, y + 1);
        }
        public Color GetColor()
        {
            return this.color;
        }
        public Position[] GetPosition()
        {
            return this.position;
        }
        public Position GetPosition(int i)
        {
            return this.position[i];
        }
        public Position[] GetOldPosition()
        {
            return this.oldPosition;
        }
        public Position GetOldPosition(int i)
        {
            return this.oldPosition[i];
        }
    }
    // ... more classes
}
share|improve this question
    
Check for the snippet creation walkthrough on MSDN. It can help if you want to implement some properties and getter + setter over dozen of class without having to rewrite everything. –  Thierry Savard Saucier Feb 11 at 18:13
add comment

1 Answer

up vote 5 down vote accepted

Please read .NET Naming conventions, in particular interfaces should be prefixed with I (Block => IBlock).

  • Your interface doesn't include SetPosition while all classes that implement this interface do have it.
  • There is no need to declare both GetPosition() and GetPosition(int) as latter doesn't add any value to interface, can be implemented as an extension method if needed.
  • When your objects are created via parameterless constructor you get an inconsistent state (OldPosition is not initialised with Position objects)
  • You mentioned getters and setters while you don't have any properties defined. I would replace GetPosition() with read-only property, and define only one SetPosition(int,int) method as an object mutator. If you do need parameterless SetPosition you can create extension method for IBlock interface that will call SetPosition(0, 0)
  • In order to avoid repeating implementation you can consider creating a base class, so that derived classes will only need to override SetPosition(int,int).

    public interface IBlock
    {
        Color Color { get; }
        Position[] Position { get; }
        Position[] OldPosition { get; }
        void SetPosition(int x, int y);
    }
    
    public class Block : IBlock
    {
        private readonly Point[] _structure;
    
        public Color Color { get; private set; }
        public Position[] Position { get; private set; }
        public Position[] OldPosition { get; private set; }
    
        public Block(Color color, Point[] structure, int x = 0, int y = 0)
        {
            Color = color;
            _structure = structure;
            //to properly initialise both Old and current positions
            SetPosition(0, 0);
            SetPosition(x, y);
        }
    
        private Position[] BuildPosition(int x, int y)
        {
            return Array.ConvertAll(_structure, point => new Position(x + point.X, y + point.Y));
        }
    
        public void SetPosition(int x, int y)
        {
            OldPosition = Position;
            Position = BuildPosition(x, y);
        }
    }
    
    public class Line : Block
    {
        public Line(int x = 0, int y = 0)
            : base(Color.BlueViolet, new Point[] { new Point(0, 0), new Point(0, 1), new Point(0, 2), new Point(0, 3) }, x, y)
        {
    
        }
    }
    
share|improve this answer
    
You shouldn't be calling virtual members from a base class (Block) constructor. Also, since Block class is abstract the constructors should be protected not public. –  Trevor Pilley Dec 27 '12 at 11:39
    
Agree, updated answer –  almaz Dec 27 '12 at 14:48
    
i'm sorry I'm not very familiar with C# yet, but what does Point mean or do? Also can you explain the constructor for line? does that mean it defaults to x=0, y=0? I'm also new to what base means –  TMP Dec 28 '12 at 6:40
1  
Point is the structure that has 2 integer properties: X and Y. I use it to store the figure configuration (shifts from base coordinates. Constructor's x = 0 means that these parameters are optional, and if caller won't provide provide this parameter it will be defaulted to 0. As to your last question - it looks like you're new to object-oriented programming, you need to learn it. base means that we're calling constructor of the base class, see Inheritance –  almaz Dec 28 '12 at 10:29
add comment

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.