-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ben Simmons Homework 2 #9
base: main
Are you sure you want to change the base?
Conversation
Good work! For the final step of this assignment:
Walk through the two solutions, comparing one small chunk of code at a time. Things to think about as you go:
Now, to complete the assignment: Write a brief note about one or two things that stood out to you as you compare the two solutions. You can ask a question, or just name something notable. Post these comments here in this comment thread and/or as comments on individual lines of your code in the “Files changed” tab. |
for arg in self.args: | ||
arg.check_types() | ||
|
||
self.receiver.static_type().method_named(self.method_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line does nothing. I put it in while figuring things out and forgot to remove it.
self.lhs.check_types() | ||
self.rhs.check_types() | ||
if not self.rhs.static_type().is_subtype_of(self.lhs.static_type()): | ||
raise JavaTypeMismatchError("Cannot assign " + self.rhs.static_type().name + " to variable " + self.lhs.name + " of type " + self.lhs.static_type().name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is way worse than the string formatting in your solution. It works, but it's messy.
for i in range(len(self.args)): | ||
args_types.append(self.args[i].static_type().name) | ||
if not self.args[i].static_type().is_subtype_of(parameter_types[i]): | ||
raise_error = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this weird solution so I could find the list of all type names, but I totally missed that there was a _names() function that does it for me.
@@ -89,6 +91,11 @@ class JavaPrimitiveType(JavaType): | |||
Primitive types are not object types and do not have methods. | |||
""" | |||
|
|||
def is_subtype_of(self, other): | |||
if other == self: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course this is just return other == self
. I did this late at night after working for a long time on some other homework, so I wasn't thinking very clearly.
return True | ||
for supertype in self.direct_supertypes: | ||
if supertype.is_subtype_of(other): | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did this on one line. I think mine is a bit more clear, but they work the same, and it could totally be argued that yours is better. However, the any() function you used is really useful.
def __init__(self): | ||
super().__init__("void") | ||
|
||
def is_subtype_of(self, other): | ||
if other == self: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why yours didn't include this, but anyway, this is another example of return other == self
being better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mine does include it. Maybe you were looking at the “without bonus solutions” link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I missed that that's a bonus thing.
return False | ||
|
||
def method_named(self, method_name): | ||
raise NoSuchJavaMethod("Cannot invoke method " + method_name + "() on null") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used "null", while you used self.name
. self.name
should always be null, so I don't think there's a difference. Also, the is_subtype_of(self, other) method should have done return other.is_object_type
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always beware of repeating the same constant in multiple places in the code, even if you think the redundancy won’t hurt anything. (What if the language decides it’s called nil
instead?)
@@ -51,7 +51,10 @@ public List<PythonObject> getMRO() { | |||
* result (i.e. it remembers the list buildMRO() returned and keeps returning it). | |||
*/ | |||
protected List<PythonObject> buildMRO() { | |||
throw new UnsupportedOperationException("not implemented yet"); | |||
mro = new ArrayList<PythonObject>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assumption with this is that buildMRO() is only ever called by getMRO(), which I did not realize. As a result, I set mro
in here instead of using a separate variable. It won't cause any problems, but it's a bit weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The side effect here does make the code a bit brittle. You’re creating a brief window where the mro
var is a mutable collection, then immediately gets replaced with an immutable one — and making it possible to leave the mutable one there permanently by calling buildMRO
directly from somewhere else. I agree that won’t cause any problems as the code exists now, but it does open up a window for future bugs.
throw new UnsupportedOperationException("not implemented yet"); | ||
mro = new ArrayList<PythonObject>(); | ||
mro.add(this); | ||
mro.addAll(type.buildMRO()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You used getType().getMRO(), which is better because you're using the actual getters. getType() won't be any different from type
at the moment, but it might change, and using buildMRO() will be slower than getMRO() and return a mutable list.
mro.add(this); | ||
if (base != null) | ||
mro.add(base); | ||
return mro; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I shouldn't have used mro here. Also, yours is significantly more robust. Yours is this:
if(getBase() != null)
result.addAll(getBase().getMRO());
if(getType() != null)
result.addAll(getType().getMRO());
Adding the MRO of the base is obviously better than just adding the base, and it will probably break things to not add the MRO of the type. Also, using getters is generally a good idea.
No description provided.