2
\$\begingroup\$

I'm trying to create an Isometric Game in JavaScript. I think the code is pretty good, but I'd like to know if there is something to improve.

        var CameraX = 0,
            CameraY = 0;

        for(var yChunks = 0; yChunks < NumYChunks; yChunks++)
        {
            var ChunkY = yChunks * HalfChunk,
                ChunkCameraY = yChunks * 4294967296;

            for(var xChunks = 0; xChunks < NumXChunks; xChunks++)
            {

                var FinalyChunk = ChunkY + CameraY;

                var FinalxChunk = xChunks * SizeChunk,
                    ChunkCameraX = xChunks + NumXChunks + CameraX;

                var index = new Number(ChunkCameraY + ChunkCameraX);

                ctx.fillStyle = "black";
                ctx.font = "bold 13px Arial";
                ctx.fillText("ID: " + index, FinalxChunk + 10, FinalyChunk + 20);

                for(var CeldY = 0; CeldY < CeldsYChunk; CeldY++)
                {
                    var yCeld = CeldY * 40;
                    for(var CeldX = 0; CeldX < CeldsXChunk; CeldX++)
                    {
                        var NewCeldX = HalfChunk + FinalxChunk + (CeldX * 40) - yCeld - 40 ,
                            NewCeldY = (CeldX * 20) + (yCeld / 2) + (FinalyChunk) - 10 ;
                        ctx.beginPath();
                        ctx.rect(NewCeldX, NewCeldY, 80, 40);
                        ctx.stroke();
                        ctx.closePath();
                    }
                }
            }
        }
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Biggest issues are naming conventions and weird brace placement. It's like you're trying to take JavaScript and make it look like some other language. Use camelCase, not StudlyCaps. Keep braces on the same line. Of course these are just stylistic observations based on what "most" JavaScript code looks like, so I'm leaving it as a comment instead of a review. Those loops get nested pretty deeply too, maybe extract some of that out into another function. \$\endgroup\$
    – Dagg
    Aug 8, 2014 at 5:14
  • 1
    \$\begingroup\$ @Dagg You have suggested concrete improvements. Your advice should go in an answer, not a comment, even if your remarks are brief. \$\endgroup\$ Aug 8, 2014 at 7:56
  • \$\begingroup\$ what the reason behind the use of new Number? \$\endgroup\$ Aug 8, 2014 at 12:11
  • \$\begingroup\$ I try to get number bigger than << 32 bits but he returned me new numbers like 2, 3, 4, So, I was trying to get big numbers as index for my array. \$\endgroup\$ Aug 8, 2014 at 14:01

1 Answer 1

2
\$\begingroup\$

Someone posted an answer which had some great points, but they deleted it. I am only going to mention a couple of points that they made as well as some new ones, but they should definitely put their answer back up!

I am no Javascript expert but I have used it before. Despite this, I am having great difficulty understanding what your code does. You say you think the code is pretty good, so I'll have to take your word for it that it does what you are expecting it to do. It is very hard to understand, though!

I'll just go through and point out things as I see them.

    var CameraX = 0,
        CameraY = 0;

Personally for this I would just say var CameraX; and var CameraY; on separate lines. I don't see what you are really gaining by omitting the second var. I know that it is legal to do it your way in Javascript, so it's not that it's wrong, but in my opinion it just causes a potential reader of your code to have to pause and reread it to fully understand.

for(var yChunks = 0; yChunks < NumYChunks; yChunks++)

You are iterating based on yChunks, but the fact that you have it plural just makes it confusing. I would suggest simply yChunk, but I will talk about that more in the next paragraph. The name NumYChunks is also a bit confusing. Is it the max number of y chunks? If so, I would call it MaxYChunks instead.

To elaborate on my earlier point, your naming is all over the place and completely inconsistent. You say things like ChunkY, Finalychunk, yChunks, NumYChunks, all in different places and they all mean different things. This makes the logical flow of your program very hard to follow. At the very least, all of these things need to have a similar format, so if you are going to have Y capitalized and at the end of the variable, always do that. So for example FinalyChunk would be FinalChunkY. But an even better approach is to minimize the number of similar names, so try to come up with better, more descriptive names whenever possible.

A related point, I would suggest that you follow the variable naming conventions in whatever language you are writing in, so for Javascript that would be camelCase.

You use many magic numbers in the code, such as

ChunkCameraY = yChunks * 4294967296;
ctx.fillText("ID: " + index, FinalxChunk + 10, FinalyChunk + 20);
ctx.rect(NewCeldX, NewCeldY, 80, 40);

What is 4294967296? What do 10 and 20 represent? What are 80 and 40? By declaring these as variables somewhere in the code, such as rectangleXSize = 80; rectangleYSize = 40; you will be accomplishing two things. First, it will be much easier to read and understand your code. Second, you will have a much easier time trying to modify the code at a later time, because at the top of the function you will see those values declared. Otherwise you will have to search through the code to find the numbers you want to change.

The main logic of the function is where things get the most confusing, at least for me. You have a four level deep for-loop structure, which is a sign that the design needs to be reevaluated. Any time you have to have that many for loops to accomplish something, there is usually a better approach. Additionally, I would break as much as possible out of the for loops and into smaller functions. This will improve the readability of your code and make the logic easier to follow and easier to change later. If you use descriptive function names, it will also be self documenting.

Finally I will echo the other (now deleted) answer and say that you should have more comments in your code. You might even be forgiven for a deeply nested for loop if you have comments that explain what is happening at each stage. At the very least, it will be a lot easier for you to understand in the future if you ever try to look at the code again!

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your post, I know that the names of my variables aren't really understandable, y always try to find a way to make good names but I can't. So the magic number is 2 << 32 but i try to do (yChunks << 32) but javascript has not long numbers and I tryed with the value of 2 << 32 that is 4294967296 and it works, I don't know but yes, and why 32 bits? So I could have 4294967296 Chunks on my Horizontal that is too big like "infinite". \$\endgroup\$ Aug 9, 2014 at 0:04
  • \$\begingroup\$ I don't think there is anything inherently wrong with what you did, but I think you should have had a comment explaining why, or define that number as a variable with a name that would make it understandable. In this case, probably both. \$\endgroup\$
    – bazola
    Aug 15, 2014 at 20:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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