INDAPlus21 / INDAPlus21/ogronman-assembly

Pass

Open
#1 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
0
Forks
0
PR merge metrics
No merged PRs in 30d

Description

**Very well done Oskar!**

I'm waiting, and waiting, and waiting... and weeeee!

Here are some notes on your code:

1) Although a love a good one-liner, good code design also requires code to be readable. Readability can be achieved with comments, and/or properly with formatting and structure.

_Your code_:
```java
for(int i = 1; i < body.size(); i++){
body.get(i).setP(body.get(i-1).getP());
}
body.get(0).setP(new Point(body.get(0).getP().x+10*body.get(0).getDirection()[0], body.get(0).getP().y+10*body.get(0).getDirection()[1]));
```
_With formatting and comments_:
```java
// Left shift snake body one step.
for(int i = 1; i < body.size(); i++){
body.get(i).setP(body.get(i-1).getP());
}
// Set new head at position relative the first body part.
body.get(0).setP(
new Point(
body.get(0).getP().x + 10*body.get(0).getDirection()[0],
body.get(0).getP().y + 10*body.get(0).getDirection()[1]
)
);
```

2) `ArrayList` already has a function perfect for this method. See: https://docs.oracle.com/javase/8/docs/api/java/util/ArrayList.html#add-int-E-

_Using Java standard library_:
```java
// Add new head at position relative the first body part.
body.add(0,
new Point(
body.get(0).getP().x + 10*body.get(0).getDirection()[0],
body.get(0).getP().y + 10*body.get(0).getDirection()[1]
)
);
```

3) `ArrayList` is a bad choise of data structure as the array has to shift all elements on every `updateBody`, i.e. a linear operation. I would suggest using a `LinkedList`, as `addFirst` would be constant. See: https://docs.oracle.com/javase/8/docs/api/java/util/LinkedList.html
4) Your solution will run slow due to douplicate calculations. For example this case with `code.substring(0, 3)`. Also, what does this method acctually do?

_Your code_:
```java
private String checkLength(String code) {
if ((code.substring(0, 3).equals("100") || code.substring(0, 3).equals("010") /*...*/)) {
//...
code = code.substring(0, 3) + temp + code.substring(3);

} else if ((code.substring(0, 3).equals("111")) /*...*/) {
if (/*...*/) {/*...*/}
else {
//...
code = code.substring(0, 3) + temp + code.substring(3);
}

}
//...

return code;
}
```
_Without douplicate calculations_:
```java
private String checkLength(String code) {

String a = code.substring(0, 3);
//...

if (a.equals("100") || a.equals("010") /*...*/) {
//...
code = a + temp + code.substring(3);

} else if (a.equals("111") /*...*/) {
if (/*...*/) {/*...*/}
else {
//...
code = a + temp + code.substring(3);
}

}
//...

return code;
}
```

5) In comparison, some of your local variables are unnecessary. In your case, I even found a zombie assignement loop.

_Your code_:
```java
//...
else {
String temp = "";
for (int i = code.length(); i < 8; i++) {
temp += "0";
}
code = code.substring(0, 3) + temp + code.substring(3);
}
//...
if (/*...*/) {
String newCode = "";
for (int i = code.length(); i >= 8; i--) {
newCode = code.substring(0, i); // <--- !!!!!
}
code = newCode;
}
```
_Without temporary variables_:
```java
//...
else {
code = code.substring(0, 3);
for (int i = code.length(); i < 8; i++) {
code += "0";
}
code += code.substring(3);
}
//...
if (/*...*/) {
code = code.length() >= 8 ? code.substring(0, 8) : new String();
}
```

6) Why do you compile numerics in string format? String operations are expensive.

_Your code_:
```java
private HashMap inst = new HashMap() {
{
put("add", "000");
//...
}
};
//...
public byte compile(String[] src) {
String compiled = "";

for (int i = 0; i < src.length; i++) {
if (inst.containsKey(src[i].toLowerCase())) {
compiled += inst.get(src[i]);
} else {
try {
compiled += decToBin(src[i]);
} catch (java.lang.NumberFormatException e) {

}
}
}

compiled = checkLength(compiled);

int convert = Integer.parseInt(compiled, 2);
byte returnByte = (byte) convert;
return returnByte;
}
//...
private String decToBin(String s) throws java.lang.NumberFormatException {
int dec = Integer.parseInt(s);
String bin = Integer.toBinaryString(dec);
return bin;
}
```
_Suggestion (not tested)_:
```java
private HashMap inst = new HashMap<>() {
{
put("add", 0b000);
//...
}
};
//...
public byte compile(String[] src) {
byte compiled = 0;
byte ptr = 8;

for (int i = 0; i < src.length; i++) {
if (inst.containsKey(src[i].toLowerCase())) {
if (src[i][1] == 't') {
compiled |= inst.get(src[i]) << ptr - 2;
ptr -= 2;
} else {
compiled |= inst.get(src[i]) << ptr - 3;
ptr -= 3;
}
} else {
try {
num = Byte.parseByte(src[i]);
compiled |= num << (num / 2);
} catch (java.lang.NumberFormatException e) {
// Handle syntax error.
}
}
}

return compiled;
}
```

7) Spagetti make Viola sad :(

_Your code_:
```java
try {
//...
} catch (java.lang.NumberFormatException e) {

}
```

8) Once again, why strings? Your emulator are string based. A byte-based emulator would require less conversions without sacrificing readability.

Thanks for your hard work!

Contributor guide

No contributing guide indexed for this repository

Research direction

The review discusses updateBody, checkLength, compile, decToBin, and the emulator, but names no files or tests. First inspect those entry points and confirm with the maintainer which refactor is in scope; done should be defined as the agreed change plus passing project checks.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
compilers
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.