RoomModel class

Question

I have coded a quick room model system, let me run you should how it exactly works

X = 1 square
0 = block square
1 = 1 level higher
2 = 2 levels higher 
and so on...

Example, a simple room model structure would be like below:

XXXXXXXXXX0XX
XXXXXXXXXX0XX
XXXXXXXXXX0XX
XXXXXXXXXX0XX
XXXXXXXXXX0XX
XXXXXXXXXX0XX
XXXXXXXXXX0XX
XXXXXXXXXX0XX
XXXXXXXXXX0XX
XXXXXXXXXX0XX
XXXXXXXXXX0XX
XXXXXXXXXX0XX

I am just looking for any way to improove it as I am self taught and would like to learn more about how C# can be optimized for better performance. Here is my code:

using System;

namespace Sahara.Base.Game.Rooms
{
    internal class RoomModel
    {
        private readonly bool _clubOnly;
        private readonly int _doorX;
        private readonly int _doorY;
        private readonly double _doorZ;
        private readonly int _doorDirection;
        private readonly string _modelHeightmap;
        private readonly int _modelSizeX;
        private readonly int _modelSizeY;
        private readonly int _wallHeight;
        private readonly short[,] _squareFloorHeight;
        private readonly byte[,] _squareSeatRotation;
        private readonly SquareState[,] _squareState;
        private readonly string _modelFurniMap;
        private readonly bool _publicPool;
        private readonly byte[,] _roomModelEffects;

        public RoomModel(bool clubOnly, int doorPositionX, int doorPositionY, int wallHeight, double doorPositionZ, int doorDirection, string modelHeightMap, string modelFurniMap, string poolMap)
        {
            _doorX = doorPositionX;
            _doorY = doorPositionY;
            _doorZ = doorPositionZ;
            _doorDirection = doorDirection;
            _wallHeight = wallHeight;
            _modelHeightmap = modelHeightMap.ToLower();
            _modelFurniMap = modelFurniMap;

            if (poolMap != string.Empty)
            {
                _publicPool = true;
                _roomModelEffects = new byte[_modelSizeX, _modelSizeY];
            }

            var temporaryHeightmap = _modelHeightmap.Split(Convert.ToChar(13));

            _modelSizeX = temporaryHeightmap[0].Length;
            _modelSizeY = temporaryHeightmap.Length;
            _clubOnly = clubOnly;
            _squareState = new SquareState[_modelSizeX, _modelSizeY];
            _squareFloorHeight = new short[_modelSizeX, _modelSizeY];
            _squareSeatRotation = new byte[_modelSizeX, _modelSizeY];

            for (var y = 0; y < _modelSizeY; y++)
            {
                var line = temporaryHeightmap[y];
                line = line.Replace("\r", "");
                line = line.Replace("\n", "");

                var x = 0;
                foreach (var modelSquare in line)
                {
                    if (modelSquare == 'x')
                    {
                        _squareState[x, y] = SquareState.Blocked;
                    }
                    else
                    {
                        _squareState[x, y] = SquareState.Open;
                        _squareFloorHeight[x, y] = ParseModelSquare(modelSquare);
                    }
                    x++;
                }
            }
        }

        public static short ParseModelSquare(char input)
        {

            switch (input)
            {
                case '0':
                    return 0;
                case '1':
                    return 1;
                case '2':
                    return 2;
                case '3':
                    return 3;
                case '4':
                    return 4;
                case '5':
                    return 5;
                case '6':
                    return 6;
                case '7':
                    return 7;
                case '8':
                    return 8;
                case '9':
                    return 9;
                case 'a':
                    return 10;
                case 'b':
                    return 11;
                case 'c':
                    return 12;
                case 'd':
                    return 13;
                case 'e':
                    return 14;
                case 'f':
                    return 15;
                case 'g':
                    return 16;
                case 'h':
                    return 17;
                case 'i':
                    return 18;
                case 'j':
                    return 19;
                case 'k':
                    return 20;
                case 'l':
                    return 21;
                case 'm':
                    return 22;
                case 'n':
                    return 23;
                case 'o':
                    return 24;
                case 'p':
                    return 25;
                case 'q':
                    return 26;
                case 'r':
                    return 27;
                case 's':
                    return 28;
                case 't':
                    return 29;
                case 'u':
                    return 30;
                case 'v':
                    return 31;
                case 'w':
                    return 32;

                default:
                    throw new FormatException("The input was not in a correct format: input must be between (0-k)");
            }
        }
    }
}

If you've created multiple accounts, please ask for them to be merged: codereview.stackexchange.com/help/merging-accounts — BCdotWEB, 14 hours ago

tinstaafl · Answer 1 · 2017-02-01 15:34:50Z

One suggestion I'd make:

Instead of having so many fields in your class, it becomes much easier to write your code, if you break out related fields into sub classes e.g:

private class Door
{
    public int X;
    public int Y;
    public double Z;
    public int Direction;
}

t3chb0t · Answer 2 · 2017-02-01 15:32:48Z

public static short ParseModelSquare(char input)

This could use some math to throw away those cases because char can be implicitly converted to int. See also Implicit Numeric Conversions Table.

public static short ParseModelSquare(char input)
{
    if ('0' <= input && input <= '9') 
    {
        return (short)(input - '0');
    }

    if ('a' <= input && input <= 'k')
    {
        const int numbersOffset = 10;
        return (short)(input - 'a' + numbersOffset);
    }

    throw new FormatException(..);
}

denis · Answer 3 · 2017-02-01 15:38:37Z

You can get rid of the long switch case like this:

public static short ParseModelSquare(char input)
{
    if (char.IsDigit(input))
    {
        return (short) (input - 48);
    }
    if (char.IsLetter(input))
    {
        return (short) (input - 87);
    }
    throw new FormatException("The input was not in a correct format: input must be between (0-k)");
}

You can remove all of those variables:

private readonly int _wallHeight;
private readonly short[,] _squareFloorHeight;
private readonly byte[,] _squareSeatRotation;
private readonly string _modelFurniMap;
private readonly bool _clubOnly;
private readonly int _doorX;
private readonly int _doorY;
private readonly double _doorZ;
private readonly int _doorDirection;
private readonly bool _publicPool;
private readonly byte[,] _roomModelEffects;

But I don't see any use of the things done in the constructor you can probably just leave it empty except for the SquareState, unless you have more code.

BCdotWEB · Answer 4 · 2017-02-01 15:29:29Z

What's the point of a RoomModel class with only private fields?

This is waaay too many arguments: public RoomModel(bool clubOnly, int doorPositionX, int doorPositionY, int wallHeight, double doorPositionZ, int doorDirection, string modelHeightMap, string modelFurniMap, string poolMap). Instead, have publicly available properties so your code becomes much clearer:

var roomModel = new RoomModel
{
   ClubOnly = clubOnly,
   DoorPositionX = doorPositionX
   // etc.
}

This also avoids errors.

Your constructor is 50+ lines. That's far too long, IMHO. Some of it should be moved to private methods, but I'd urge you to keep RoomModel lean and move the logic in your constructor to a separate "Creator" class.

I'd make ParseModelSquare() into a class of its own as well. (Why is this a public method?) Perhaps also look at replacing it with a Dictionary<char, short>() and finding a more elegant way to fill it instead of a 70 line switch.

I think you can replace this:

var line = temporaryHeightmap[y];
line = line.Replace("\r", "");
line = line.Replace("\n", "");

with this:

var line = temporaryHeightmap[y].Replace(Environment.NewLine, string.Empty);

This conveys more clearly what you are doing.

asked	today
viewed	493 times
active	today

current community

your communities

more stack exchange communities

RoomModel class

4 Answers 4

Your Answer

Not the answer you're looking for? Browse other questions tagged c# or ask your own question.

Linked

Hot Network Questions

current community

your communities

more stack exchange communities

RoomModel class

4 Answers 4

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c# or ask your own question.

Linked

Related

Hot Network Questions