A CodeReview.SE style review

Subtitle: I'm Not Going To Bother Rewriting Your Code Better Because It's Java.

Read my other, less mean but equally nitpicky reviews of other people's code:

"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."

"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..."

-- Jack Diedrich, CPython core library developer

First, a minor note on style preference which has an application.

In C, it's useful to give function declarations in this format:

/^(?:(?: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+{/

This (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.

That is, function declarations look like this:

static inline __CONST_FUNC __PURE_FUNC void func (void* ptr) {

and calls to it look like this:

func(&a_thing);

The 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.

On with the review.

import java.util.*;

These blanket imports are ugly and defeat the very main point of Java's verboseness -- readability. _{although Java is verbose to the point of unreadability...}

It'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.

Student student;
School school;

public Transcript( Student student, School school) {
  this.student = student;
  this.school = school;
}

For 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.

void printTranscript() {
  System.out.println("\nInitializing Transcript....\n\n\n");
  System.out.println(student.address);
  System.out.println(student.name);
  System.out.println(student.phoneNumber);
  System.out.println(student.dob);
  System.out.println(student.guardian);
  System.out.println("\n");
  System.out.println(school.name);
  System.out.println(school.address);
  System.out.println(school.phoneNumber);
  System.out.println(school.email);
  System.out.println("\n");
  // ... snip ...
}

This 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.

Moreover, 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!?

Printing 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).

So please, for the love of Richard Stallman and Linus Torvalds and everything that is holy, don't do that. Use a god damn StringBuilder.

int year = 9;

I 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.

When 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.

For 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.

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:

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. Note that int variables are still signed when declared but unsigned arithmetic is now possible by using those methods in the Integer class.

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.

This 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.

And another thing, you just defined two variable names of disparate type with very similar names within two lines...

int year = 9;
for(HashMap<Course, Character> years : student.grades)

Yes, their scopes are different, but don't do that.

System.out.println("--------------------Grade-" + year + "-----------------" );
System.out.println("--------Class----------------Credits--Grade--");

Don'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.

Then, 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.

The 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.

Java is unsuprisingly lacking in this aspect in its stubborn cross-platformness, but you can check System.Console() for null and that may work well enough.

That is, if stdout is a teletypewriter, don't write crap. K?

if (course.name.length() < 8) {
  System.out.println("\t"+course.name + "\t\t\t" +
                     course.credits + "\t" +
                     years.get(course));

} else if (course.name.length() > 15) {
  System.out.println("\t"+course.name + "\t" +
                     course.credits + "\t" +
                     years.get(course));

} else {
  System.out.println("\t"+course.name + "\t\t" +
                     course.credits + "\t" +
                     years.get(course));
}

More magic numbers, but this time, there's not a hint of indication from the code why they exist, unlike with int year.

I 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.

Write a dedicated method for this ugliness, or please, for the love of the aformentioned Gods, format it like so:

if ( your_obfuscated_condition ) {
  System.out.println(
    "\t" + course.name +
    "\t\t\t" + course.credits +
    "\t" + years.get(course)
  );
}

Doesn't Java have raw string literals? Even C# and C++ can do that. What a shame.

year++;

Okay, 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.

This just seems smelly to me...

System.out.println("Calculated GPA over 4 years: "+ String.format("%.01f", student.calculateGPA()));

Line 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.

Lines 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.

In this case, it should be formatted like:

System.out.println(
  "Calculated GPA over 4 years: " +
  String.format("%.01f", student.calculateGPA())
);

The same goes for the first lines of void Main(String[] args).

Finally, the last few lines of void printTranscript and its outer for loop have mismatched indentation. For shaaaame.

School kennett = new School("Kennett High School", "409 Eagles' Way, North Conway, NH", "603-356-4343", "info@khs.com");
Student dmr84 = new Student("Dan Richardi", "123 You Wish, North Conway, NH", "555-555-5555", "01/29/1984", "Gandalf");

Again, 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.

ArrayList<Course> freshmanCourses = new ArrayList<Course>();
freshmanCourses.add(new Course("English 9", 1.0f));
freshmanCourses.add(new Course("Algebra 1", 1.0f));
freshmanCourses.add(new Course("Biology w/lab", 1.0f));
freshmanCourses.add(new Course("American History", 1.0f));
freshmanCourses.add(new Course("Drawing", 1.0f));
freshmanCourses.add(new Course("Martial Arts 1", 0.5f));
freshmanCourses.add(new Course("MS Office", 0.5f));

ArrayList<Course> sophomoreCourses = new ArrayList<Course>();
sophomoreCourses.add(new Course("English 10", 1.0f));
sophomoreCourses.add(new Course("Algebra II", 1.0f));
sophomoreCourses.add(new Course("Chemistry w/lab", 1.0f));
sophomoreCourses.add(new Course("World History", 1.0f));
sophomoreCourses.add(new Course("Latin I", 1.0f));
sophomoreCourses.add(new Course("Martial Arts II", 0.5f));
sophomoreCourses.add(new Course("Piano", 0.5f));

ArrayList<Course> juniorCourses = new ArrayList<Course>();
juniorCourses.add(new Course("English 11", 1.0f));
juniorCourses.add(new Course("Algebra II", 1.0f));
juniorCourses.add(new Course("Marine Biology w/lab", 1.0f));
juniorCourses.add(new Course("American Government", 0.5f));
juniorCourses.add(new Course("Economics", 0.5f));
juniorCourses.add(new Course("Latin II", 1.0f));
juniorCourses.add(new Course("Web Design", 1.0f));

ArrayList<Course> seniorCourses = new ArrayList<Course>();
seniorCourses.add(new Course("English 12", 1.0f));
seniorCourses.add(new Course("Calculus", 1.0f));
seniorCourses.add(new Course("Physics w/lab", 1.0f));
seniorCourses.add(new Course("Photography", 0.5f));
seniorCourses.add(new Course("Yearbook", 0.5f));
seniorCourses.add(new Course("Driver's Education", 0.5f));
seniorCourses.add(new Course("Studio Art", 1.0f));
seniorCourses.add(new Course("Piano", 0.5f));

Ughhhh, god, please, no.

Seems 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.

You 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.

Either way I refuse to believe this is the best way to achieve this.

dmr84.freshmanYear = dmr84.assignGrades(freshmanCourses);
dmr84.sophomoreYear = dmr84.assignGrades(sophomoreCourses);
dmr84.juniorYear = dmr84.assignGrades(juniorCourses);
dmr84.seniorYear = dmr84.assignGrades(seniorCourses);

The primary thing that annoyed me when I ran your program was just Holy cats, how many numbers am I going to have to enter?

Again, each function should do one thing, do it well, and return its output to another function.

So not only do you do too much in Main but the functions you call from Main do too much and the Scanner call, which is what I was interested in looking at because I was wondering how many times I would have to enter "99", is not even in main, it's in a function with a totally unrelated and non-descriptive name!

The logic so far needs some refactoring to make it easier to follow.

Transcript dmrTrans = new Transcript(dmr84, kennett);
dmrTrans.printTranscript();

dmrTrans, seniorCourses, juniorCourses etc should all be declared at the top of their enclosing scope and assigned asap.

Again, with the printTranscript nonsense. Call System.out.println(dmrTrans.toString()) instead, okay?

public School( String name,
                String address,
                String phoneNumber,
                String email)
{
  this.name = name;
  this.address = address;
  this.phoneNumber = phoneNumber;
  this.email = email;
}

Heh, public School, heh...

Okay, first of all, the formatting relationship between the closing paren and the opening brace is awful. This is another reason braces go on the same line as their related statements.

Second... really, these four properties and no methods or executable code are a whole god damn object that's going to be compiled into a .class file?? Use a HashMap property of Transcript objects instead, since Java apparently doesn't have structures...

Scanner input = new Scanner(System.in);
Character grade;
// ... snip ...
grade = input.next().charAt(0);

The Gods at Oracle gave you a beautiful Scanner with which to read as much data as your verbose and broken heart desires, and you read one damned byte. Why not allow numbers, or even signs on the characters? That would simplify some other code...

Moreover, you have abused the power vested in you by the Scanner class, by the simple act of not telling me, the user, what I'm supposed to enter, and because I assume grades are numbers, I entered 100 and expected it to work.

Consider a println that tells me what are and aren't valid inputs.

On another note, doesn't check that grade turns out to be in abcdfABCDF, and indeed doesn't check that grade is a latin alphabetical glyph either.

Java wonderfully supports Unicode UTF-8 throughout, so your program considers the number 1, the Japansese Katakana glyph あ, as well as the letter Z to all be valid letter grades, which is almost certainly not what you want.

Finally, this code's enclosing method has a god-awful undescriptive name.

for(Course course : courses)

Oh, for crying out loud. Course is a type, course is a Course, and courses is an ArrayList<Course>.

Better identifiers, please.

public float calculateGPA() { /* ... */ }

I don't know Java, but I think there's some way to declare properties with getters and setters.

In Python (which may have gotten it from Java) I think it looks like this:

class Whatever():
  @property
  def gpa(self):
    return self.gpa

  @gpa.setter
  def gpa(self, value):
    self.gpa = value

you get the idea.

for (HashMap<Course, Character> gradeYear : grades)
{
  for (Course course : gradeYear.keySet())
  {
    switch(gradeYear.get(course))
    {
      case 'A':
        total += 4.0f;
        courseCount++;
        break;
      case 'B':
        total += 3.0f;
        courseCount++;
        break;
      case 'C':
        total += 2.0f;
        courseCount++;
        break;
      case 'D':
        total += 1.0f;
        courseCount++;
      default:
        courseCount++;
        break;

    }
  }
}
return total / courseCount;

A for loop for this? What is this, C? Maybe it is.

If this were Java 8, this could be solved with the map and reduce methods on ArrayList objects.

Then, you can just convert the character letters to their number values (by subtracting 65 from each) and reduce using +.

In fact, this can probably be changed to

for (HashMap<Course, Character> gradeYear : grades) {

  courseCount += gradeYear.size();

  for (Course course : gradeYear.keySet()) {
    total += (float) 65 - gradeYear.get(course);
  }
}
return total / courseCount;

There is a size() method on HashMaps, it does what you'd expect.

grades.addAll(Arrays.asList(freshmanYear, sophomoreYear, juniorYear, seniorYear));

Uhhhhhhhhhhh... what?

Local variables.

What does this even say? To grades, add the arrays freshmanYear, sophomoreYear, juniorYear, and seniorYear as lists? What does that mean?

Also, line too long (87 > 77 chars)

class Course
{
  String name;
  float credits;

  public Course(String name,
                float credits)
  {
    this.name = name;
    this.credits = credits;
  }
}

Again... just... use a HashMap.

Conclusion: Where's the typedef keyword?!

drichardi / Transcript-v2.0

A CodeReview.SE style review #3

Subtitle: I'm Not Going To Bother Rewriting Your Code Better Because It's Java.