INDAPlus21 / INDAPlus21/ogronman-assembly
Pass
- 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