{"action":"opened","issue":{"url":"https://api.github.com/repos/drichardi/Transcript-v2.0/issues/3","repository_url":"https://api.github.com/repos/drichardi/Transcript-v2.0","labels_url":"https://api.github.com/repos/drichardi/Transcript-v2.0/issues/3/labels{/name}","comments_url":"https://api.github.com/repos/drichardi/Transcript-v2.0/issues/3/comments","events_url":"https://api.github.com/repos/drichardi/Transcript-v2.0/issues/3/events","html_url":"https://github.com/drichardi/Transcript-v2.0/issues/3","id":186812673,"number":3,"title":"A CodeReview.SE style review","user":{"login":"catb0t","id":3238748,"avatar_url":"https://avatars.githubusercontent.com/u/3238748?v=3","gravatar_id":"","url":"https://api.github.com/users/catb0t","html_url":"https://github.com/catb0t","followers_url":"https://api.github.com/users/catb0t/followers","following_url":"https://api.github.com/users/catb0t/following{/other_user}","gists_url":"https://api.github.com/users/catb0t/gists{/gist_id}","starred_url":"https://api.github.com/users/catb0t/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/catb0t/subscriptions","organizations_url":"https://api.github.com/users/catb0t/orgs","repos_url":"https://api.github.com/users/catb0t/repos","events_url":"https://api.github.com/users/catb0t/events{/privacy}","received_events_url":"https://api.github.com/users/catb0t/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2016-11-02T14:16:59Z","updated_at":"2016-11-02T14:16:59Z","closed_at":null,"body":"## Subtitle: I'm Not Going To Bother Rewriting Your Code Better Because It's Java.\r\n\r\nRead my other, less mean but equally nitpicky reviews of other people's code:\r\n\r\n* [Dynamic class instancing (with conditional parameters and methods) based on a dictionary](http://codereview.stackexchange.com/a/118702/87163)\r\n* [Slice chunking in Go](http://codereview.stackexchange.com/a/118829/87163)\r\n* [Successful use of `strtol()` in C](http://codereview.stackexchange.com/a/143268/87163)\r\n* [Gravity problem solver in Factor](http://codereview.stackexchange.com/a/123061/87163)\r\n\r\n---\r\n\r\n> \"I learned about Separation of Concerns, Decoupling, Encapsulation, Implementation Hiding, uhm, I haven't used those words in 15 years since I graduated. Anytime you hear someone use those words, they're gonna pull a fast one on you. It just doesn't come up.\"\r\n\r\n> \"If you see a class with two methods [other than the constructor] in it, it's not a class. Don't define new Exceptions when you don't need to, and you don't need to...\"\r\n\r\n> -- Jack Diedrich, CPython core library developer\r\n\r\n---\r\n\r\n**First, a minor note on style preference which has an application**.\r\n\r\nIn C, it's useful to give function *declarations* in this format:\r\n\r\n```js\r\n/^(?:(?:static|inline)*\\s+[a-zA-Z0-9][a-zA-Z0-9_*]+|[a-zA-Z0-9][a-zA-Z0-9_*]+)\\s+[a-zA-Z0-9][a-zA-Z0-9_]+\\s+\\(.*\\)\\s+{/\r\n```\r\n\r\nThis (outdated and recently untested) regex says that a declaration consists of possibly the words `static` and `inline`, a type identifier `_like90This1` followed by a function name and pointer type `fu_9Unc**` **followed by whitespace** followed by a parameter list, then an open brace and a newline.\r\n\r\nThat is, function declarations look like this:\r\n\r\n```c\r\nstatic inline __CONST_FUNC __PURE_FUNC void func (void* ptr) {\r\n```\r\n\r\nand calls to it look like this:\r\n\r\n```c\r\nfunc(&a_thing);\r\n```\r\n\r\nThe whitespace in the declaration but *not* in the calls makes it easy to use only a regex to find, without doubt as to which, a call *or* its declaration, **but not both at the same time**.\r\n\r\n**On with the review.**\r\n\r\n---\r\n\r\n```java\r\nimport java.util.*;\r\n```\r\n\r\nThese blanket imports are ugly and defeat the very main point of Java's verboseness -- readability. <sub> although Java is verbose to the point of *un*readability...</sub>\r\n\r\nIt's like in Python -- either use `import time` or `from time import strftime` but don't do `from time import *` because it's unspecific and pollutes the global namespace.\r\n\r\n---\r\n\r\n```java\r\nStudent student;\r\nSchool school;\r\n\r\npublic Transcript( Student student, School school) {\r\n  this.student = student;\r\n  this.school = school;\r\n}\r\n```\r\n\r\nFor the size of this code, there's little to no reason to have these pointless parameters. OOP people love to make new classes for everything, but I see no reason to not just pass the raw data -- it would simplify the implementation and shorten the code.\r\n\r\n---\r\n\r\n```java\r\nvoid printTranscript() {\r\n  System.out.println(\"\\nInitializing Transcript....\\n\\n\\n\");\r\n  System.out.println(student.address);\r\n  System.out.println(student.name);\r\n  System.out.println(student.phoneNumber);\r\n  System.out.println(student.dob);\r\n  System.out.println(student.guardian);\r\n  System.out.println(\"\\n\");\r\n  System.out.println(school.name);\r\n  System.out.println(school.address);\r\n  System.out.println(school.phoneNumber);\r\n  System.out.println(school.email);\r\n  System.out.println(\"\\n\");\r\n  // ... snip ...\r\n}\r\n```\r\n\r\nThis method is called `printTranscript` yet you are doing a lot of printing which is unequivocally *not* related to the actual Transcript itself. Consider defining a `toString()` method on School objects which returns a String of this data.\r\n\r\nMoreover, this printTranscript method should be named `toString()`, and should not print directly to `stdout` but rather return the data as a String. **How would you like it if all the builtin Object objects' `toString()` methods printed directly instead of being kind and giving you the data!?**\r\n\r\nPrinting directly to stdout should be done for low-level debugging, which should not be an issue in Java because there are no null pointers (even though there are).\r\n\r\nSo please, for the love of Richard Stallman and Linus Torvalds and everything that is holy, don't do that. Use a god damn StringBuilder.\r\n\r\n---\r\n\r\n```java\r\nint year = 9;\r\n```\r\n\r\nI was going to complain about would you *please* consider the implications of arithmetic types of things, but then I remembered Java doesn't have unsigned types, because someone was a lazy bitch.\r\n\r\nWhen you move to C, *please please please* consider the sign and size of the type you're using for the kind of data you want to store.\r\n\r\nFor instance, do not store actual years as signed. Just don't do it, okay? Your language surely has a `Date` data type -- in C it's `struct tm` from `time.h` as specified by System V rev4, C89, C99, POSIX.1-2001, SUSv2, ad nauseam.\r\n\r\n> However, as of Java SE 8, there are a few new methods in the Integer class which allow you to use the int data type to perform unsigned arithmetic:\r\n\r\n> In Java SE 8 and later, you can use the int data type to represent an unsigned 32-bit integer, which has a minimum value of 0 and a maximum value of 2^32-1. Use the Integer class to use int data type as an unsigned integer. Static methods like compareUnsigned, divideUnsigned etc have been added to the Integer class to support the arithmetic operations for unsigned integers.\r\nNote that int variables are still signed when declared but unsigned arithmetic is now possible by using those methods in the Integer class.\r\n\r\n`year` is an awful variable name not because of its length but because it's *wrong* -- the year is *not* 9, but the student's grade year *is*. A name like `gradeYear` or something would suffice.\r\n\r\nThis variable name makes this a magic number. Magic numbers are annoying because I've got no idea what it is. Comments are not needed if the code's logic and identifiers explain itself.\r\n\r\n**Also** you just defined two variable names of disparate type with *very* similar names within two lines. Don't *do* that.\r\n\r\n---\r\n\r\n```java\r\nSystem.out.println(\"--------------------Grade-\" + year + \"-----------------\" );\r\nSystem.out.println(\"--------Class----------------Credits--Grade--\");\r\n```\r\n\r\nDon't do this, okay? I did this when I was 15 in Python in Programming I, then I realised that `binary_operator *` is defined commutatively between `str` and `int` so I can do `'-' * 20` or whatever.\r\n\r\nThen, I grew up a little more and learned about the philosophies of Unix upon which all modern software is regrettably built (yes, even Wingdows). The most important of these is that everything is text and thus, programs which write text should do so in a fluffless way.\r\n\r\nThe primary implication is *not* \"don't make stdout pretty\" but perhaps, either don't write text not meant to be parsed *or* if you do this, at least use the Java equivalent of [`isatty`](http://unix.stackexchange.com/a/314369/136107).\r\n\r\nJava is unsuprisingly lacking in this aspect in its stubborn cross-platformness, but you [can check `System.Console()` for `null`](http://stackoverflow.com/a/23419451/4532996) and that may work well enough.\r\n\r\nThat is, if stdout is a teletypewriter, don't write crap. K?\r\n\r\n---\r\n\r\n```java\r\nif (course.name.length() < 8) {\r\n  System.out.println(\"\\t\"+course.name + \"\\t\\t\\t\" +\r\n                     course.credits + \"\\t\" +\r\n                     years.get(course));\r\n\r\n} else if (course.name.length() > 15) {\r\n  System.out.println(\"\\t\"+course.name + \"\\t\" +\r\n                     course.credits + \"\\t\" +\r\n                     years.get(course));\r\n\r\n} else {\r\n  System.out.println(\"\\t\"+course.name + \"\\t\\t\" +\r\n                     course.credits + \"\\t\" +\r\n                     years.get(course));\r\n}\r\n```\r\n\r\nMore magic numbers, but this time, there's not a *hint* of indication from the code why they exist, unlike with `int year`.\r\n\r\nI don't even know why it matters that the length is greater than 15 or less than 8, but that ugly `println` doesn't make it very clear either.\r\n\r\nWrite a dedicated method for this ugliness, or please, for the love of the aformentioned Gods, format it like so:\r\n\r\n```java\r\nif ( your_obfuscated_condition ) {\r\n  System.out.println(\r\n    \"\\t\" + course.name +\r\n    \"\\t\\t\\t\" + course.credits +\r\n    \"\\t\" + years.get(course)\r\n  );\r\n}\r\n```\r\n\r\nDoesn't Java have raw string literals? Even C# and C++ can do that. What a shame.\r\n\r\n---\r\n\r\n```java\r\nyear++;\r\n```\r\n\r\nOkay, maybe clever because it just counts up from the starting year, but you set that starting year in *stone*, so either get it passed as a parameter or refactor your code to not work like... that. Ugh, my head genuinely hurts from this nonsense.\r\n\r\nThis just seems smelly to me...\r\n\r\n---\r\n\r\n```java\r\nSystem.out.println(\"Calculated GPA over 4 years: \"+ String.format(\"%.01f\", student.calculateGPA()));\r\n```\r\n\r\nLine 58 is 103 characters long -- this is too long. Limit lines to ~77 chars because of some people's screen resolution or just my editor's multiwindow mode.\r\n\r\nLines that are hundreds of characters long either do too much with too many expressions and are thus unreadable, or contain long strings which should be broken up with backslashes, etc.\r\n\r\nThe same goes for the lines in `void Main(String[] args)`.\r\n\r\nFinally, the last few lines of `void printTranscript` and its outer for loop have mismatched indentation. For shaaaame.\r\n\r\n```java\r\nSchool kennett = new School(\"Kennett High School\", \"409 Eagles' Way, North Conway, NH\", \"603-356-4343\", \"info@khs.com\");\r\nStudent dmr84 = new Student(\"Dan Richardi\", \"123 You Wish, North Conway, NH\", \"555-555-5555\", \"01/29/1984\", \"Gandalf\");\r\n```\r\n\r\nAgain, lines 62 and 63 are way too long -- either shorten the number of arguments to the constructor, refactor the logic to not need pointless objects, assign the arguments to local variables or break the constructor call over many lines.\r\n\r\n---\r\n\r\n```java\r\nArrayList<Course> freshmanCourses = new ArrayList<Course>();\r\nfreshmanCourses.add(new Course(\"English 9\", 1.0f));\r\nfreshmanCourses.add(new Course(\"Algebra 1\", 1.0f));\r\nfreshmanCourses.add(new Course(\"Biology w/lab\", 1.0f));\r\nfreshmanCourses.add(new Course(\"American History\", 1.0f));\r\nfreshmanCourses.add(new Course(\"Drawing\", 1.0f));\r\nfreshmanCourses.add(new Course(\"Martial Arts 1\", 0.5f));\r\nfreshmanCourses.add(new Course(\"MS Office\", 0.5f));\r\n\r\nArrayList<Course> sophomoreCourses = new ArrayList<Course>();\r\nsophomoreCourses.add(new Course(\"English 10\", 1.0f));\r\nsophomoreCourses.add(new Course(\"Algebra II\", 1.0f));\r\nsophomoreCourses.add(new Course(\"Chemistry w/lab\", 1.0f));\r\nsophomoreCourses.add(new Course(\"World History\", 1.0f));\r\nsophomoreCourses.add(new Course(\"Latin I\", 1.0f));\r\nsophomoreCourses.add(new Course(\"Martial Arts II\", 0.5f));\r\nsophomoreCourses.add(new Course(\"Piano\", 0.5f));\r\n\r\nArrayList<Course> juniorCourses = new ArrayList<Course>();\r\njuniorCourses.add(new Course(\"English 11\", 1.0f));\r\njuniorCourses.add(new Course(\"Algebra II\", 1.0f));\r\njuniorCourses.add(new Course(\"Marine Biology w/lab\", 1.0f));\r\njuniorCourses.add(new Course(\"American Government\", 0.5f));\r\njuniorCourses.add(new Course(\"Economics\", 0.5f));\r\njuniorCourses.add(new Course(\"Latin II\", 1.0f));\r\njuniorCourses.add(new Course(\"Web Design\", 1.0f));\r\n\r\nArrayList<Course> seniorCourses = new ArrayList<Course>();\r\nseniorCourses.add(new Course(\"English 12\", 1.0f));\r\nseniorCourses.add(new Course(\"Calculus\", 1.0f));\r\nseniorCourses.add(new Course(\"Physics w/lab\", 1.0f));\r\nseniorCourses.add(new Course(\"Photography\", 0.5f));\r\nseniorCourses.add(new Course(\"Yearbook\", 0.5f));\r\nseniorCourses.add(new Course(\"Driver's Education\", 0.5f));\r\nseniorCourses.add(new Course(\"Studio Art\", 1.0f));\r\nseniorCourses.add(new Course(\"Piano\", 0.5f));\r\n```\r\n\r\nUghhhh, god, please, no.\r\n\r\nSeems to be a lot of duplication here. You could write a function (not a method on Transcript objects, because no `this` use) which returns an `ArrayList<Course>` from a varargs number of `Course` arguments. It's depressing that the `add` method or constructor of `ArrayList` objects doesn't already define such an overload.\r\n\r\nYou could also define two arrays, one which holds course names and the other their weights and make courses using loops. This would be much better except there are no references or array literals in this language. For fucks' sake.\r\n\r\nEither way I refuse to believe this is the best way to achieve this.\r\n\r\n(continued...)"}}
{"actor":{"display_login":"catb0t"}}