8
\$\begingroup\$

I already improved my program with the suggestions of @Mast. As requested by him I re-upload the code in English so we can check it out together.

But before I'll explain a little bit what this program does. As my brother is a hydrogeologist, he needed a program that could transform spin of an hydrometric reel into speed (here is a brief explanation with a photo under "Measuring with small hydrometric reel"), to calculate the flow of the river.

This has to go on a Raspberry Pi, that he will bring on the field to simplify his work. That's why I chose Python and appJar to do this work.

Here is the code improved as discussed in this other question:

import csv
#import RPi.GPIO as GPIO #library for raspberry pins
import time
from appJar import gui
l=[]
class measure: #All the measures that needs to be into the csv file
    global l
    def fileName(self):#Station where you do the measures have a code, and we call the filename with that code
        fileName=app.getEntry("e1") +'.csv'
        return fileName
    def vertical(self):#The point where you measure the speed of the river
        vertical=float(app.getEntry("e2"))
        return vertical
    def depthTot(self):
        depthtot=float(app.getEntry("e3"))#The depth of the river where you are doing the measure
        return depthtot
    def edgeDist(self): #Distance from the edge of the river
        e=float(app.getEntry("e2"))
        return e
    def hydroDepth(self): # Depth where you have the hydrometric reel
        hydrodepth=float(app.getEntry("e4"))
        return hydrodepth
    def changeMs(self): #Function that clears the entries for the measures, as you do many measures on the same station
        app.clearEntry("e2")
        app.clearEntry("e3")
        app.clearEntry("e4")
        app.clearLabel("e5")
        app.showButton("Start measure")
        app.setFocus("e2")
    def changeDepth(self): #This clears only the last entry, as if the river is a lot deep you need to do more measures on the same vertical
        prof=float(app.getEntry("e4"))
        v=app.getLabel("e5")
        l.append(prof)
        l.append(v)
        app.clearEntry("e4")
        app.showButton("Start measure")
        app.setFocus("e4")
    def changeStation(self):#Clears all entries, when you change station
        app.clearEntry("e1")
        app.clearEntry("e2")
        app.clearEntry("e3")
        app.clearEntry("e4")
        app.clearLabel("e5")
        app.showButton("Start measure")
        app.setFocus("e1")
    def hydroReel(self): #Gets the size of the Hydrometric reel
        size=app.getOptionBox("HydroReel")
        return size
    def measurementTime(self): #The measure of speed needs to be over a period of time, so you have less error
        t=app.getOptionBox("Secondi misurazione")
        return float(t)

class calculations:
    def speed(self,hydroreel,spin): #this is the function that calculates the speed from the spins of the hydroreel
        v=0
        spin_1s=0
        if spin=='':
            v=''
            return v
        spin=float(spin)
        spin_1s=spin/30
        if spin_1s==0:
            v=0
            return v
        if mulinello=='125':
            if spin_1s<1.98:
                v=(1.93+(31.17*spin_1s))/100
                return v
            elif spin_1s<10.27:
                v=(0.19+(32.05*spin_1s))/100
                return v
            else:
                v=(-14.09+(33,44*spin_1s))/100
                return v
        elif mulinello=='80':
            if spin_1s<1:
                v=(2.8+(31.34*spin_1s))/100
                return v
            else:
                v=(0.82+(33.32*spin_1s))/100
                return v
        elif mulinello=='50':
            if spin_1s<1.74:
                v=(1.23+(24.73*spin_1s))/100
                return v
            else:
                v=(-0.42+(25.68*spin_1s))/100
                return v
        elif mulinello=='30':
            if spin_1s<1.16:
                v=(1.90+(10.57*spin_1s))/100
                return v
            else:
                v=(2.26+(10.26*spin_1s))/100
                return v
    def spinCounter(self,t): #this function measures the spins through raspberry
        spin=input('Inserire numero di spin') # here i used this input for the spins because i don't use the raspberry every time
        final_t = time.time()+t
        return spin
        """GPIO.setmode(GPIO.BOARD)
        GPIO.setup(32,GPIO.IN)

        #set up a counter
        spin = 0

        #set up a variable for reed activation
        reed_state = 0
        print("Misurazione in corso...")
        t_fine = time.time()+temp_mis
        #while loop until 30s
        while time.time()<t_fine:
        #check if reed newly activated
            if GPIO.input(32) == 1 and reed_state == 0:
                #turn on LED. Set reed_state to 1. Add to counter .
                reed_state = 1
                spin = spin + 1
            #pause to debounce
            time.sleep(.01)
            #check if reed released
            if GPIO.input(32) == 0 and reed_state == 1:
                # set reed_state to 0
                reed_state = 0

            #now that loop has finished, print the final count
        return spin """
def startMeasure(self):     #this is the function triggered to start the measure of speed
        c=calculations()
        m=measure()
        v=c.speed(m.hydroReel(),c.spinCounter(m.measurementTime()))
        v=round(v,4)
        l=[m.hydroDepth(),v]
        app.setLabel("e5",v)
        app.hideButton("Start measure")
file1=''
def insertMeasure(self): #this function inserts all the data into a csv file
    global file1,mis_0,l
    m=measure()
    myFile = open(m.fileName(),'a')
    with myFile:
        writer = csv.writer(myFile,lineterminator='\n')
        if file1!=m.fileName():
            firstLine = ["Vertical", "Edge distance", "Depth"]+["HydroDepth","Speed"]*5
            writer.writerow(firstLine)
            file1=m.fileName()
            mis_0=float(m.edgeDist())
        depth=float(app.getEntry("e4"))
        v=app.getLabel("e5")
        l.append(depth)
        l.append(v)
        writer.writerow([m.vertical(),(m.vertical()-mis_0),m.depthTot()]+l)
        l=[]

def putInto(Button):
    focus=app.getFocus()
    entry=app.getEntry(focus)
    entry=entry+Button
    app.setEntry(focus,entry)
def press(Button):
    putInto(Button)


app=gui()   #this is the grapich core of the program
app.setTitle("Water app")
app.setFont(size=12, family="Calibri")

app.addLabel("l1", "Measures",0,0)
app.addLabel("l2", "Speed",5,0)


app.addEntry("e1",1,0)
app.addEntry("e2",2,0)
app.addEntry("e3",3,0)
app.addEntry("e4",4,0)
app.addLabel("e5","",6,0)

app.addButton("Start measure",startMeasure,6,0)

app.addButton("Inserisci misura",insertMeasure,4,1)
app.addButton("Altra profondità",measure.changeDepth,3,1)
app.addButton("Cambia misura",measure.changeMs,2,1)
app.addButton("Cambia stazione",measure.changeStation,1,1)

app.setEntryDefault("e1", "Station number")
app.setEntryDefault("e2", "Vertical n°")
app.setEntryDefault("e3", "Total depth")
app.setEntryDefault("e4", "Hydro reel distance")
app.setGuiPadding(15, 5)
app.addLabelOptionBox("Reel size", ['125', '80', '50','30'],5,1)
app.addLabelOptionBox("Seconds of measurement", ['15', '30', '60','120','180'],6,1,2)

app.setLabelFont(size=13,weight="bold")
app.setEntryWidths(["e1","e2","e3","e4"], 20)
app.setEntryRelief("e1", "raised")
app.setEntryRelief("e2", "raised")
app.setEntryRelief("e3", "raised")
app.setEntryRelief("e4", "raised")

app.addButtons([["1","2","3"],["4","5","6"],["7","8","9"],["0",".","O"]],press,1, 2, 3,4)
app.addButtons([["A","B","C","D"],["E","F","G","H"],["I","L","M","N"],["P","Q","R","S"],["T","U","V","Z"]],press,1,5,4,5)

app.go()

I wanted to ask if there is anything else I could do to improve this little program (could we add the tag appJar to simplify searching?).
EDIT: is it possible to add the data to an excel or Libre office Calc sheet instead of csv? Because it would be more useful.

\$\endgroup\$
  • \$\begingroup\$ Hello! I think many comments from codereview.stackexchange.com/questions/191526/… (including style) apply to this piece of code as well. Also, all the code in """triple-quoted string""" seems pretty weird... \$\endgroup\$ – Josay Apr 9 '18 at 12:26
  • \$\begingroup\$ In that piece I simply count how many times the reel spins, as it works as a simple on/off switch. I already tested it on the raspberry and it works great. As styling I am currently working on it. @Josay \$\endgroup\$ – Lorenz Bonat Apr 9 '18 at 12:30
3
\$\begingroup\$
  1. There's a bunch of style/spacing issues. I suggest reading PEP8. That said, if you're the only one maintaining this code, do what you want.

  2. Methods of the form:

    def getSomething(self):
        something=...
        return something
    

    are better written just as:

    def getSomething(self):
        return ...
    

    For instance:

    def fileName(self): #Station where you do the measures have a code, and we call the filename with that code
        return app.getEntry("e1") +'.csv'
    
  3. Similarly, you can just return expressions instead of having to assign them to a variable first. There's a lot of:

    v=...
    return v
    

    that could be just:

    return ...
    
  4. Class and function documentation is free if you move your comments a little. If you change that to:

    def fileName(self):
        """Station where you do the measures have a code, 
            and we call the filename with that code"""
        return app.getEntry("e1") +'.csv'
    

    then you can see those comments from the command-line interface via the help() command. Also, using triple quotes instead of # lets you use multi-line comments.

\$\endgroup\$
  • \$\begingroup\$ I am seeing the styling through pep8. Didn't thought that there were so many rules. \$\endgroup\$ – Lorenz Bonat Apr 10 '18 at 7:06
  • \$\begingroup\$ @LorenzBonat And you can automatically check against some of those rules automatically. For example, there's the pep8 package, and the (inferior) online check. \$\endgroup\$ – Mast Apr 10 '18 at 14:34
3
\$\begingroup\$

Classes

I see both too many and too little classes in your code. On one hand, you have the calculations class, where the self is used nowhere, but on the other hand you have the global variable app, where you do a lot of operations on, and which is changed a lot in your measure class. I would change the measure class, to do all the operations of the GUI. Since it does no measures any more,

calculations

There is a lot of repeated code in your speed calculation with just changed constants. This can be done a lot easier using a dict

from collections import OrderedDict
HYDROREELS = OrderedDict()
HYDROREELS['125'] = (
    (1.98, 1.93, 31.17),
    (10.28, 0.19, 32.05),
    (float('inf'), -14.09, 33.44),
)
HYDROREELS['80'] = (
    (1, 2.8, 31.34),
    (float('inf'), .82, 33.32),
)
HYDROREELS['50'] = (
    (1.74, 1.23, 24.73),
    (float('inf'), -0.42, 25.68),
)
HYDROREELS['30'] = (
    (1.16, 1.90, 10.57),
    (float('inf'), 2.26, 10.26),
)


def speed(hydroreel, spin):
    """this is the function that calculates the speed from the spins of the hydroreel"""
    if spin in {0, ''}:
        return spin
    spin = float(spin)
    spin_1s = spin / 30

    coefficients = HYDROREELS[hydroreel]
    for limit, c0, c1 in coefficients:
        if spin < limit:
            return (c0 + (c1 * spin_1s)) / 100

adding a new hydroreel is now as easy as adding a value to the dict

GUI

Instead of manually adding each label, which can be a real pain when you want to rename something or translate something, you can work with a dict of label names, and a list of entries

LABEL_NAMES = {
    'reel_size': 'Reel Size',
    'measurement_interval': 'Seconds of measurement',
    'measure_start': 'Start Measure',
    'measure_save': 'Inserisci misura',
    'measurement_change': "Cambia misura",
    'depth_change': 'Altra profondità',
    'station_number': "Station number",
    'vertical_no': "Vertical n°",
    'depth_total': 'Total depth',
    'hydroreel_distance': "Hydro reel distance",
    'station_change': "Cambia stazione",
}

ENTRIES = ['station_number', 'vertical_no', 'depth_total', 'hydroreel_distance']

Now the class with the GUI

class MyGUI:
    def __init__(self):
        self.app = gui()

        self._populate()

        self.current_file = ''
        self.mis_0 = 0
        self.l = []

Here we use the instance variables current_file, l and mis_0 instead of the global variables in your version

and instead of doing all the adding of the buttons and labels in the global namespace, we have a populate method

def _populate(self):

    self.app.setTitle('Water app')
    self.app.setFont(size=12, family='Calibri')

    self.app.addLabel('l1', 'Measures', 0, 0)
    self.app.addLabel('l2', 'Speed', 5, 0)

    self.app.addLabelOptionBox(LABEL_NAMES['reel_size'], HYDROREELS.keys(), 5, 1)
    self.app.addLabelOptionBox(LABEL_NAMES['measurement_interval'], ['15', '30', '60', '120', '180'], 6, 1, 2)

    self.app.setLabelFont(size=13, weight='bold')
    self.app.setGuiPadding(15, 5)

    for i, entry in enumerate(ENTRIES, 1):
        self.app.addEntry(entry, i, 0)
        self.app.setEntryRelief(entry, 'raised')
        self.app.setEntryDefault(entry, LABEL_NAMES[entry])

    self.app.addLabel('current_speed', "", 6, 0)
    self.app.setEntryWidths(ENTRIES, 20)

    self.app.addButton(LABEL_NAMES['measure_start'], self.start_measure, 6, 0)
    self.app.addButton(LABEL_NAMES['measure_save'], self.save_measure, 4, 1)
    self.app.addButton(LABEL_NAMES['depth_change'], self.change_depth, 3, 1)
    self.app.addButton(LABEL_NAMES['measurement_change'], self.clear_entries, 2, 1)
    self.app.addButton(LABEL_NAMES['station_change'], self.change_station, 1, 1)
    self.app.addButtons([['1', '2', '3'], ['4', '5', '6'], ['7', '8', '9'], ['0', '.', 'O']], self.press, 1, 2, 3,
                        4)
    self.app.addButtons(
        [['A', 'B', 'C', 'D'], ['E', 'F', 'G', 'H'], ['I', 'L', 'M', 'N'], ['P', 'Q', 'R', 'S'],
         ['T', 'U', 'V', 'Z']],
        self.press, 1, 5, 4, 5)

Off course this method can be split into smaller, methods which are easier to grasp and improve.

As you can see, the labels are replaced by the value in the LABEL_NAMES, and the manual sizes of the hydro_reels is taken from the keys of HYDROREELS, and the e1 etc. are replaced by more descriptive labels, placed in a list

fields

def file_name(self):  #
    """Station where you do the measures have a code, and we call the filename with that code"""
    return self.app.getEntry(ENTRIES[0]) + '.csv'

clear entries

since we have the entries in a list now, clearing them all becomes as simple as

def change_station(self):  #
    """Clears all entries, when you change station"""
    entries = ENTRIES
    for entry in entries:
        self.app.clearEntry(entry)
    self.app.clearLabel('current_speed')
    self.app.showButton(LABEL_NAMES['measure_start'])
    self.app.setFocus(entries[0])

The complete code can be found here

and is called like this

app = MyGUI()
app.go()

There are still a lot of options to make this code cleaner and avoid some repitition, but this puts you on the path already

\$\endgroup\$
  • \$\begingroup\$ in regards to app = MyGUI() app.go() - you even don't need that, just MyGUI().go(). \$\endgroup\$ – C. Harley Apr 11 '18 at 1:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.