Skip to content

Refarctoring#2

Open
divalbanerjee wants to merge 10 commits intomasterfrom
refarctoring
Open

Refarctoring#2
divalbanerjee wants to merge 10 commits intomasterfrom
refarctoring

Conversation

@divalbanerjee
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
Member

@keerthi-sekar keerthi-sekar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Love the format now, if you can add a readme to explain the motor and cell attributes so other members can easily reference that while looking at the code that would be great. The calculations itself (can be written by hand) is also needed but for the PDR.

Code wise I only saw 2 things: cell has 2 constructors that initialize the properties in an almost identical way and just was wondering if it is needed if the only input form is by file. Also, keep in mind exception handling in terms of the flag enabled, the return value should also return a "null" value like -1 to match the print statement so the user knows what values were not calculated. (Just seeing that the return value happens even if it enter the if)

Comment thread src/cell.py
capacity = -1
maxContinousDischarge = -1
internalResistance = -1
remainingCapacity = -1 #TODO: Rename this
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been renamed? (Has the todo been complete?) If not, Please complete and update the PR.

Comment thread src/cell.py
maxContinousDischarge = -1
internalResistance = -1
remainingCapacity = -1 #TODO: Rename this
usableCapacity = -1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe put a comment below or above the definitions so other people can understand the difference between these attributes (usable vs remaining) If there is a lot of theoretical calculations and background please add documentation referencing the code

Comment thread src/cell.py

def findEnergyDensity(self):
if(FLAGS_ENABLED == 1):
if(self.capacity <= 0):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If capacity is <= 0 then the function should return with the print statement. From looking at this code, it looks like it still does the calculation after printing the statement. (Return a -1 to show that it wasn't possible)

Comment thread src/motor.py
@@ -0,0 +1,97 @@
#file --motor.py--

class motor(object):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again define these values in the separate document/readme like cell so other members can understand conceptually each attribute.

Comment thread src/cell.py
self.remainingCapacity = capacity

@classmethod
def basicCell(self, ratedVoltage, maxAmperage):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of having Empty and basic? Based on the input being a file, can't we take out one of them? The attributes are still the same from what I am seeing

Comment thread src/pack.py
self.cellList.append(newCell)

def getEnergyRequired(self):
if(FLAGS_ENABLED == 1):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each function returning a value with Flag Enabled, if it's not possible set the return value to -1 to indicate to the user that the value could not be read. (In addition to the print statement)

Comment thread main.py
return self.cellsInParallel

#Gets capacity in Ah
def getCapacity(self):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an exception handling reminder - if the value is not possible, after printing set the value to a null value like -1 so the user knows it could not be calculated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants