Pablo Aguirre

Junilu Lacar wrote:The name FileProcessor is also vague whereas the JavaDocs are very specific. If the JavaDocs fully explain the reason for this class' existence, then maybe you can find a name that better reflects that purpose.

I decided to use another class mainly for practice purposes. In this case what would you suggest for the name of the class? Something more specific? FileEditor, FileCopy, FileCopyAndAppend ?

Thanks!!

Junilu Lacar wrote:

feel free to tear it apart

Ok...
A program should tell a story. This program doesn't tell its story very well. It starts out being very vague about what it's doing by creating a FileProcessor object and then calling that object's copyFile() method. Nothing else in that code gives the reader any kind of context on what they should expect, except the error handling boilerplate around the call to copyFile(). What file is being copied? Where is it getting copied to? The code is very mysterious-looking.

Ok, starting from here. What would be your suggestion about how to achieve this? My first modification was to add info in a javadoc block.

/** * This is an exercise from CodeGym, Lesson 18 - Introduction to Streams * * This program reads 3 filenames from the console * It copies the contents of the second file to the first file * It subsequently appends the contents of the third to the first file * * @author pablosouza * */ public class Solution {    public static void main(String[] args)  {        FileProcessor fileProcessor = new FileProcessor();        try {            fileProcessor.copyFile();        } catch (IOException e) {            System.out.println("General I/O exception: " + e.getMessage());            e.printStackTrace();        }    } }

Junilu Lacar wrote:

Pablo Aguirre wrote:I'm replicating a question I posted on stackexchange so here's the link if you want to check out some opinions.

Thanks for informing people that you've posted the same question elsewhere. You missed adding the link though so here's the one I found: https://codereview.stackexchange.com/questions/243005/architecture-and-error-handling-in-simple-java-program

Thank you for taking the time to reply! Yeah, I forgot to add the link, sorry. I'll rethink the code with your reply in mind. I had another comprehensive reply on stackexchange that suggests a couple of the same things. I'll probably come back soon with more questions about your reply :-D

I'm replicating a question I posted on stackexchange so here's the link if you want to check out some opinions. Code review is in my opinion the single most valuable way to improve my code so any feedback is appreciated.

----
I'm trying to get my basic coding principles right but there's still a long way to go. This is an exercise for CodeGym. They logic itself isn't very hard and I passed the exercise easily but I'd like my code reviewed with the following in mind:

- In terms of the general architecture, I wasn't quite sure if I needed two methods. It felt tidier to me to have a method for the first task (copy) and one for the second (append) but I couldn't figure out a way to avoid calling the second method from the first.

- Should read the inputs from the console in the main method and pass the result to the class? Or it totally depends on the context of the application I'm working for?

- About error handling, I got completely lost. After doing some research I just decided to let the caller handle the error but I'm sure there's some stuff missing. I wasn't sure for example if I should have two try blocks on the first method but it also depended on the answer to the first point (architecture), so I gave up.

- It's my first time using Javadocs, so I'm not sure what I wrote is enough.

- I know "Solution" is not the ideal name for the class, but I used the class created by CodeGym.

Please feel free to tear it apart

Thanks in advance!

public class Solution {    public static void main(String[] args)  {        FileProcessor fileProcessor = new FileProcessor();        try {            fileProcessor.copyFile();        } catch (IOException e) {            System.out.println("General I/O exception: " + e.getMessage());            e.printStackTrace();        }    }

import java.io.*; /** *  This class reads 3 filenames from console, *  copies content from the second file to the first and subsequently *  appends content from third file to first file. */ public class FileProcessor {    // Study comment: Solution using "var" didn't work in this IntelliJ    /**     * Reads inputs, copies second file to first and calls method to append file     * @throws IOException     */    public void copyFile() throws IOException {        String outputFile;        String sourceToCopy;        String sourceToAppend;         try (BufferedReader reader = new BufferedReader(new InputStreamReader(System.in))) {             System.out.println("Enter a filename for the output:");             outputFile = reader.readLine();             System.out.println("Enter the path of the file to be copied");             sourceToCopy = reader.readLine();             System.out.println("Enter the path of the file to be appended");             sourceToAppend = reader.readLine();         }        try (FileOutputStream out = new FileOutputStream(outputFile);             FileInputStream copy = new FileInputStream(sourceToCopy)) {            byte[] buffer = new byte[1024];            while (copy.available() > 0) {                int count = copy.read(buffer);                out.write(buffer, 0, count);            }        }        appendFile(sourceToAppend, outputFile);    }    /**     *     * @param source file to append     * @param output final result file     * @throws IOException     */    public void appendFile(String source, String output) throws IOException {        try (FileOutputStream appended = new FileOutputStream(output, true);             FileInputStream toAppend = new FileInputStream(source)        ) {            byte[] buffer = new byte[1024];            while (toAppend.available() > 0) {                int count = toAppend.read(buffer);                appended.write(buffer, 0, count);            }        }    } }

Zachary Griggs wrote:

I don't think countCommas should declare that it throws an IOException
You catch IOException here. You could also move the lines above (that may throw IOException) into this try block.
       } catch (IOException e) {            e.printStackTrace();
So it can't throw any IOException.

I'm doing my next exercise and trying to close a method using the finally block and I'm getting an error telling me I should declare the method throws an IOException.

I tried moving the declarations into the try block but then the variable can't be reached. Any thoughts? Here's the link for the image:

https://imgur.com/a/fqD5LIN

Edit:

I fixed it using the code below but not sure if it's correct.

} finally {            try {                input.close();            } catch (IOException e) {                e.printStackTrace();            }            try {                output.close();            } catch (IOException e) {                e.printStackTrace();            }        }

Zachary Griggs, Fantastic stuff, thank you so much for the comments, I'll definitely think about all of that for writing code from now on. From your comments I realised I have zero consideration for readability.

I've been trying to follow advice from this amazing forum and from now on my objective isn't only to pass the tests, but also write using good practices. So I'll be posting some exercises to be critiqued:

For this exercise, my changes were:
1 - Avoid writing all the code in the main method
2 - Pay closer attention to exception throwing
3 - Close the stream properly

Just FYI, the "Solution" class was given beforehand by the exercise.

Please tear my code apart!

/* Read a file name from the console. Count the number of commas (',') in the file. Display the number. Close the streams. Hint: You need to compare with the ASCII code for ','. Requirements: 1. The program should read a file name from the console. 2. Use a FileInputStream to read from the file. 3. The program should display the number of commas in the read file. 4. The stream used to read the file must be closed. */ public class Solution {    public static void main(String[] args) throws IOException {        countCommas();    }    static void countCommas() throws IOException {            FileInputStream input = null;            BufferedReader reader = new BufferedReader(new InputStreamReader(System.in));            String name = reader.readLine();        try {            input = new FileInputStream(name);            int count=0;            while(input.available() > 0) {                // Reads next byte from the file                int i = input.read();                //count commas                if (i == 44) {                    count++;                }            }            //Display comma count            System.out.println(count);        } catch (FileNotFoundException e) {            e.printStackTrace();        } catch (IOException e) {            e.printStackTrace();        } finally {            if (input != null) {                input.close();            }        }    } }

Thanks everyone for the amazing answers! I'll come back with more exercise solutions for you guys to tear apart soon haha

Hi! Thanks for your reply. I really appreciate your feedback.

The exercise is from CodeGym. It's the best resource I found so far because it's exercise and projects based and that's the way I learn best. The student's solutions are checked automatic by some IntelliJ plugin they have. Not sure you can open if you're not logged in : Codegym Exercise


Yeah, my solution is too complicated, I used a solution based on a HashMap and it simplified things. But I'll try to create an object as you suggested.

I'm using .close() based on this : http://tutorials.jenkov.com/java-io/fileinputstream.html. Why shouldn't i use it?

Thanks!!

Hey folks!

I came across this exercise where I have the most frequent bytes on file and display them.  I have two questions:

1 - Should I use my time to try to solve this or this is just a mind twister that won't be helpful in the future?

2 - I tried an approach without HashMaps. I created a byte array, sorted it, found the most frequent item and the frequency and then looped again to add any items with Max Frequency to a result ArrayList<byte>. But the exercise is not passing the automatic check. Here's the code. Is the code doing what I think it's doing? Adding all the most frequent bytes to the result ArrayList?

Any comments about bad practices or different approaches are also appreciated:

public class Solution {    public static void main(String[] args) throws Exception { //        BufferedReader reader = new BufferedReader(new InputStreamReader(System.in)); //        String fileName = reader.readLine();        File file = new File("/Users/pablosouza/Downloads/test.txt");        FileInputStream fis = null;        List <Byte> result = new ArrayList<>();        try {            fis = new FileInputStream(file);            byte fileContent[] = new byte[(int)file.length()];            fis.read(fileContent);            Arrays.sort(fileContent);            int max = 1;            byte temp = fileContent[0];            int count = 1;            for (int i = 1; i<fileContent.length; i++) {                if (fileContent[i] == fileContent[i-1]) {                    count++;                } else {                    if (count > max) {                        max = count;                        temp = fileContent[i-1];                    }                    count = 1;                }            }            System.out.println(temp); //  /Users/pablosouza/Downloads/test.txt            int max1 = 1;            byte temp2 = fileContent[0];            for (int i = 1; i<fileContent.length; i++) {                if (fileContent[i] == fileContent[i-1]) {                    count++;                    if (count == max) {                        result.add(temp2);                    }                } else {                    if (count > max1) {                        max1 = count;                        temp2 = fileContent[i-1];                    }                    count = 1;                }            }            for (byte b: result){                System.out.print(b+" ");            }        } catch (IOException e) {            System.out.println("exception reading file");        } finally {            if (fis != null) {                fis.close();            }        }    }    }

Junilu Lacar wrote:

Does this clear up some things for you?

I'll need a bit of time to study your post, haha. Then I'll get back to you. Thanks so much!

Question, what's a good code simulator for Java for people to work in a project simultaneously? Something equivalent to stackblitz for Angular. I think it's a great tool for study.

Junilu Lacar wrote:

Pablo Aguirre wrote:I'm still a bit confused with the names of the variables. ... I'm assuming the y,x variables in the if statement are the ones in the game object, not the new ones created to run the loop?

If you're referring to the y and x here:

for (int y = ... ) {
   for (int x = ... ) {

then yes, they are the ones referred to in the if-statements. The gameObject x and y fields are qualified: gameObject.x and gameObject.y.

Shouldn't they have a different name to make it easier to visualize? (a,b) or whatever. Or what am I missing? Again thanks a lot for the invaluable replies.

They could have different names but I doubt (a, b) would make their intent any clearer. 'neighborX' and 'neighborY' maybe but that's a little wordy. In this case, I don't actually mind y and x that much.

I could suggest at least one different way that is more object-oriented but that would take us a whole 'nother path that you might not be ready to go down right now.

After creating this little test I finally understood that writing for (int a = gameObject.y - 1; a <= gameObject.y + 1; a++)  because the number in the matrix depends on the number of the coordinates.

private List<GameObject> getNeighbors(GameObject gameObject) {        int count=0;        List<GameObject> result = new ArrayList<>();        for (int y = gameObject.y - 1; y <= gameObject.y + 1; y++) {            for (int x = gameObject.x - 1; x <= gameObject.x + 1; x++) {                if (y < 0 || y >= SIDE) {                    System.out.println("I'm the skipped cell y " + y);                    continue;                }                if (x < 0 || x >= SIDE) {                    System.out.println("I'm the skipped cell x " + x);                    continue;                }                if (gameField[y][x] == gameObject) {                    System.out.println("I'm valid cell x y " + y + x);                    continue;                }                count++;                System.out.println("I've been added to result " + gameField[y][x].y + gameField[y][x].x);                result.add(gameField[y][x]);            }        }        return result;    }

Tim Holloway wrote:OK. The object is to create a List og GameObject's that are adjacent to a specified GameObject. This is done by scanning the following potential matrix, which is overlaid on the board matrix:

(-1, -1)  | (0, -1) | (+1, -1) ----------+---------+--------- (0, -1)   | (-----) | (0, -1) ----------+---------+--------- (-1, +1)  | (0, +1) | (+1, +1)
Note that the center cell is omitted, since that's where the reference GameObject itself lies.

We increment the x and y co-ordinates from relative -1 to relative +1, indexing the x co-ordinate the fastest. Or, in other words, scan by row, then column going left-to-right and top-to-bottom.

Hower, the gameboard is of finite dimensions, so co-ordinate positions that fall off the edge of the board (board co-ordinates of less than 0 or more than the board width and height) are ignored, since they don't exist and would throw an ArrayIndexException if you tried to use them.

Given those constraints, the eligible cells are each checked for the presence of another GameObject, and if present, that GameObject is added to the returned list. If there are no adjacent objects, an empty list will be returned.

I'm still a bit confused with the names of the variables. After your explanation I understand what the loop is doing, but when trying to simulate the loop starting on int y = -1, x=-1 (if y<0 || y>=side). I'm assuming the y,x variables in the if statement are the ones in the game object, not the new ones created to run the loop? Shouldn't they have a different name to make it easier to visualize? (a,b) or whatever. Or what am I missing? Again thanks a lot for the invaluable replies.

Junilu Lacar wrote:It looks like the code assumes that all cells on the gameField have gameObjects in them. Maybe an empty cell is also represented by a special kind of GameObject instance.

Thanks for all the replies, I'm still trying to make sense of the code as it's slightly above my head. I think yes, the previous code assumes there are no empty cells. In a different method all the cells are filled a GameObject:

private void createGame() {        for (int y = 0; y < SIDE; y++) {            for (int x = 0; x < SIDE; x++) {                boolean isMine = getRandomNumber(10) < 1;                if (isMine) {                    countMinesOnField++;                }                gameField[y][x] = new GameObject(x, y, isMine);                setCellColor(x, y, Color.ORANGE);            }        }        countMineNeighbors();    }