-2
\$\begingroup\$

My friends in the same project told me that I needed to improve this code's complexty, so I tried but couldn't figure out how to do more.. They also said that I needed to change my coding style and background... This is my code and I have it's picture.

case Terrain ->{
            ConclusiveTileSet tileSet=GameLogic.getGameLogic().getTileSet();
            ArrayList list=new ArrayList();
            for(int i=0;i<2305;i++){
                list.add(0);
            }
            list.set(2065,15);
            list.set(2080,10);
            list.set(2074,5);
            int maxX=Math.max(blockPos1.x(),blockPos2.x());
            int minX=Math.min(blockPos1.x(),blockPos2.x());
            int maxY=Math.max(blockPos1.y(),blockPos2.y());
            int minY=Math.min(blockPos1.y(),blockPos2.y());
            int Z=blockPos1.z();
            for(int i=minX;i<=maxX;i++){
                for (int j=minY;j<=maxY;j++){
                    int d=GameLogic.getGameLogic().getWorldLogic().blockAt(new VectorInt3D(i,j,Z)).getId();
                    if (d==2074||d==2080||d==2065){
                        int h= (int) list.get(d);
                        for(int k=99-h;k>=0;k--){
                            GameLogic.getGameLogic().getWorldLogic().erase(new VectorInt3D(i,j,k+h));
                            GameLogic.getGameLogic().getWorldLogic().writeEdit(new VectorInt3D(i,j,k+h),GameLogic.getGameLogic().getWorldLogic().blockAt(new VectorInt3D(i,j,k)).getId(),tileSet.getVoxelData(block.get()).getCollisionType().createDataStorage());
                        }
                        for(int k=h-1;k>=0;k--){
                            GameLogic.getGameLogic().getWorldLogic().erase(new VectorInt3D(i,j,k));
                        }
                    }
                }
            }
        }

This is the code. I created a function that builds terrain height based on the selected area's block, from highest to lowest. I really don't know about the other functions since I didn't code it, but my friends have confirmed that they are not wrong...

This is the picture of my coding.My code!!

\$\endgroup\$
4
  • 7
    \$\begingroup\$ Is the image of the code the same as the code posted into the question? If so, then please delete the image. \$\endgroup\$
    – toolic
    Commented May 18 at 15:48
  • 3
    \$\begingroup\$ State what your code does in your title, not your main concerns about it. \$\endgroup\$
    – toolic
    Commented May 18 at 15:49
  • \$\begingroup\$ Please do not upload images of code/data/errors. \$\endgroup\$
    – J_H
    Commented May 19 at 4:01
  • \$\begingroup\$ Please ask your friend about how they want the code to be formatted in your project. You'll find the correct solution much faster that way instead of trial and erroring multiple solutions suggested by random strangers from the internet. Also, we cannot help you choose a font for your editor. It is much too subjective of a topic. \$\endgroup\$ Commented May 20 at 4:40

1 Answer 1

3
\$\begingroup\$

This code is not yet of a quality that merits merging down the feature branch to main.

The Review Context helpfully explains that we have

a function that builds terrain height based on the selected area's block, from highest to lowest.

But the OP source code uses global variables to pass in what should be parameters like blockPos{1,2}.

When you take the tour it explains that

  1. Titles should describe what the code does
  2. The more code you provide the more we can help

but alas, the OP submission ignores that advice. If there is code surrounding this case clause, we should see it, or enough to understand the context.

You elected to omit any /** javadocs */ and /* comments */ which might help the Gentle Reader to understand what the code does.

Please extract helper function(s) which have appropriate parameters, such as blockPos{1,2}.

meaningful identifier

This is not helpful.

            ArrayList list = new ArrayList();

Yes, we can see it is a list, you defined it that way. Tell us what it means. Is it a list of tiles? Of non-negative integers? What do those integers mean? Does each one perhaps represent a height?

magic numbers

Is the meaning of this line supposed to be obvious?

            for (int i=0; i<2305; i++) {

Hmmm, 5 × 461. Should that ring any bells? Is it the number of licks to get to the center of a tootsie pop?

            list.set(2065, 15);
            list.set(2080, 10);
            list.set(2074, 5);
                        ...
                        for (int k = 99 - h; ... ; ... ) {

Same question. What were you thinking when you wrote that? Perhaps you believed it would be obvious, and needed no comment? Perhaps there is some ReadMe file, or other documentation, that was accidentally omitted from the stackexchange Review Context?

The trouble started with choosing a throwaway list identifier, rather than something which communicated the meaning of these values.

Consider presenting these values in sorted numeric order.

why we write code

The OP source code communicates certain bit patterns to the JVM, which can then perform automated transformations from an input pattern to a result pattern. That's not why we write code. Communicating with the machine, sending bits into a JVM and reading out result bits, is trivially easy. What we want is for those bits to have meaning.

We write code to communicate technical ideas to colleagues. The fact that the code might execute on some platform is incidental. Focus on getting your ideas in good order, and on clearly conveying them from your mind to the mind of a colleague. Exploit the various tools at your disposal, including source code techniques and natural language rhetorical techniques.

lint

            int Z = blockPos1.z();

nit: no idea why you upcased z there. Whatever.

line length

                            GameLogic.getGameLogic().getWorldLogic().writeEdit(new VectorInt3D(i,j,k+h),GameLogic.getGameLogic().getWorldLogic().blockAt(new VectorInt3D(i,j,k)).getId(),tileSet.getVoxelData(block.get()).getCollisionType().createDataStorage());

My display screen, like the screens of many of my colleagues, is of finite width, containing fewer than a million horizontal pixels in any given raster. Similarly my font uses more than a couple of pixels in the \$x\$ direction to depict each glyph.

Wow, that's a lot of characters! More than two hundred.

I didn't read them. By placing them on a single line, you told me loud and clear that you don't care if I read them. So I didn't. I did not attempt to horizontally scroll to the end.

Please format your code so humans can read it. Communicating bits to the machine is a piece of cake. Communicating technical ideas to colleagues may require a little more effort. But it's worth it.


This codebase does not appear to achieve its design objectives.

I would not be willing to delegate or accept maintenance tasks on it.

\$\endgroup\$

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