2
\$\begingroup\$

I developed a code in python that:

  • add name, age and Id information for each person,
  • search a person by name, if it exists then display a message "Found" and fill "age" and "Id" with the corresponding information, else display "not found" and fill "age" and "Id" with "---". In case of more that one name is found then a wxchoice lists all these names, after the user can choose one item in the list, then all information (age and id) related to that chosen name appears.

Note that all information are stored in a json file called "data.json".

I need to know if that is the correct way (easy to maintain, optimised, respects good practice ...) to implement that requirements:

Find below the code:

import json
import os
import wx
import wx.xrc



class ManageJson:
    def __init__(self, jsonfile):
        self.jsonfile = jsonfile
        self.content = {"Info": []}

    def read(self):
        if self.isempty() is False:
            with open(self.jsonfile, 'r') as infile:
                self.content = json.load(infile)

    def write(self):
        with open(self.jsonfile, 'w') as outfile:
            json.dump(self.content, outfile, indent=4)

    def isempty(self):
        return os.stat(self.jsonfile).st_size == 0

    def Presearch(self, pattern):
        return [data for data in self.content["Info"] if pattern in data['Name'] and pattern != '']

    def Postsearch(self, pattern):
        return [data for data in self.content["Info"] if pattern == data['Name'] and pattern != '']

    def add(self, *arg):
        self.content["Info"].append({"Name": arg[0], "Age": arg[1], "Id": arg[2]})


class ManageGui(ManageJson):

    def __init__(self, jsonfile):
        super().__init__(jsonfile)

    def exporttojson(self, *arg):
        name = arg[0]
        age = arg[1]
        Id = arg[2]
        statusbar = arg[3]
        data = super().Presearch(name.GetValue() )
        if name.GetValue() != '' and age.GetValue() != '' and Id.GetValue() != '':
            if len(data) == 0 and name.GetValue() != '':
                if super().isempty() is False:
                    super().read()
                super().add(name.GetValue(), age.GetValue(), Id.GetValue())
                super().write()
                statusbar.SetStatusText("Exporting done!")
            else:
                statusbar.SetStatusText("Element is already exist!")
        else:
            statusbar.SetStatusText("One field is empty!")

    def importfromjson(self):
        super().read()

    def Presearch(self, *arg):
        super().read()
        data = super().Presearch(arg[0].GetValue())
        print(data)
        print(len(data))
        if len(data) == 1:
            arg[4].Clear()
            f = data[0]
            arg[0].SetValue(f["Name"])
            arg[1].SetValue(f["Age"])
            arg[2].SetValue(f["Id"])
            arg[3].SetStatusText("Found")
            arg[4].Append(f["Name"])
        elif len(data) > 1:
            arg[4].Clear()
            for i in range(len(data)):
                f = data[i]
                arg[4].Append(f["Name"])
            arg[3].SetStatusText("More than one found, choose one?")
        else:
            arg[1].SetValue("---")
            arg[2].SetValue("---")
            arg[3].SetStatusText("Not Found!")

    def Postsearch(self, *arg):
        super().read()
        id = arg[4].GetSelection()
        data = super().Postsearch(arg[4].GetString(id))
        if len(data) == 1:
            f = data[0]
            arg[0].SetValue(f["Name"])
            arg[1].SetValue(f["Age"])
            arg[2].SetValue(f["Id"])
            arg[3].SetStatusText("Found")
        else:
            arg[1].SetValue("---")
            arg[2].SetValue("---")
            arg[3].SetStatusText("Not Found!")

class Frame(wx.Frame):

    def __init__(self, parent):
        wx.Frame.__init__(self, parent, id=wx.ID_ANY, title=u"Fist Application", pos=wx.DefaultPosition,
                          size=wx.Size(500, 300), style=wx.DEFAULT_FRAME_STYLE | wx.TAB_TRAVERSAL)

        self.SetSizeHints(wx.DefaultSize, wx.DefaultSize)

        self.m_statusBar2 = self.CreateStatusBar(1, wx.STB_SIZEGRIP, wx.ID_ANY)
        self.m_menubar = wx.MenuBar(0)
        self.m_file = wx.Menu()
        self.m_Exit = wx.MenuItem(self.m_file, wx.ID_ANY, u"Exit", wx.EmptyString, wx.ITEM_NORMAL)
        self.m_file.Append(self.m_Exit)

        self.m_menubar.Append(self.m_file, u"File")

        self.SetMenuBar(self.m_menubar)

        gSizer1 = wx.GridSizer(0, 2, 0, 0)

        self.m_name = wx.StaticText(self, wx.ID_ANY, u"Name", wx.DefaultPosition, wx.DefaultSize, 0)
        self.m_name.Wrap(-1)

        gSizer1.Add(self.m_name, 0, wx.ALL, 5)

        self.m_textname = wx.TextCtrl(self, wx.ID_ANY, wx.EmptyString, wx.DefaultPosition, wx.DefaultSize, 0)
        gSizer1.Add(self.m_textname, 0, wx.ALL, 5)

        self.m_age = wx.StaticText(self, wx.ID_ANY, u"age", wx.DefaultPosition, wx.DefaultSize, 0)
        self.m_age.Wrap(-1)

        gSizer1.Add(self.m_age, 0, wx.ALL, 5)

        self.m_textage = wx.TextCtrl(self, wx.ID_ANY, wx.EmptyString, wx.DefaultPosition, wx.DefaultSize, 0)
        gSizer1.Add(self.m_textage, 0, wx.ALL, 5)

        self.m_Id = wx.StaticText(self, wx.ID_ANY, u"Id", wx.DefaultPosition, wx.DefaultSize, 0)
        self.m_Id.Wrap(-1)

        gSizer1.Add(self.m_Id, 0, wx.ALL, 5)

        self.m_textId = wx.TextCtrl(self, wx.ID_ANY, wx.EmptyString, wx.DefaultPosition, wx.DefaultSize, 0)
        gSizer1.Add(self.m_textId, 0, wx.ALL, 5)

        gSizer1.Add((0, 0), 1, wx.EXPAND, 5)

        sbSizer1 = wx.StaticBoxSizer(wx.StaticBox(self, wx.ID_ANY, wx.EmptyString), wx.HORIZONTAL)

        self.m_search = wx.Button(sbSizer1.GetStaticBox(), wx.ID_ANY, u"Search", wx.Point(-1, -1), wx.DefaultSize, 0)
        sbSizer1.Add(self.m_search, 0, wx.ALL, 5)

        m_choice2Choices = [u"empty "]
        self.m_choice2 = wx.Choice(sbSizer1.GetStaticBox(), wx.ID_ANY, wx.DefaultPosition, wx.DefaultSize,
                                   m_choice2Choices, 0)
        self.m_choice2.SetSelection(0)
        sbSizer1.Add(self.m_choice2, 0, wx.ALL, 5)

        self.m_add = wx.Button(sbSizer1.GetStaticBox(), wx.ID_ANY, u"Add", wx.Point(-1, -1), wx.DefaultSize, 0)
        sbSizer1.Add(self.m_add, 0, wx.ALL, 5)

        gSizer1.Add(sbSizer1, 1, wx.ALL | wx.ALIGN_BOTTOM | wx.ALIGN_RIGHT, 5)

        self.SetSizer(gSizer1)
        self.Layout()

        self.Centre(wx.BOTH)

        # Connect Events
        self.Bind(wx.EVT_MENU, self.Exit, id=self.m_Exit.GetId())
        self.m_search.Bind(wx.EVT_BUTTON, self.search)
        self.m_choice2.Bind(wx.EVT_CHOICE, self.Display)
        self.m_add.Bind(wx.EVT_BUTTON, self.addnew)

    def __del__(self):
        pass

    # Virtual event handlers, overide them in your derived class

    def search(self, event):
        inst.Presearch(self.m_textname, self.m_textage, self.m_textId, self.m_statusBar2, self.m_choice2)

    def addnew(self, event):
        inst.exporttojson(self.m_textname, self.m_textage, self.m_textId, self.m_statusBar2)

    def Display(self, event):
        inst.Postsearch(self.m_textname, self.m_textage, self.m_textId, self.m_statusBar2,self.m_choice2)

    def Exit(self, event):
        self.Close(True)  # Close the frame.


inst = ManageGui("data.json")
app = wx.App(False)
frame = Frame(None)
frame.Show()
app.MainLoop()
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. There is proof of correctness (→CS), and there is methodical testing. Are you confident your code works as specified? \$\endgroup\$ – greybeard Nov 12 '19 at 21:33
  • \$\begingroup\$ @greybeard I did not perform any such test. But basing on my tests I can confirm that the functional behaviour is correct. what do you mean by proof of correctness (→CS) and methodical testing? \$\endgroup\$ – ABA Nov 13 '19 at 10:34
  • 2
    \$\begingroup\$ Welcome to Coe Review! I have rolled back your last edit. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Nov 13 '19 at 10:38
2
\$\begingroup\$

Ways of improving:

Naming issues

A lot of Python naming conventions violations. A proper names should be given to identifiers and functions/methods.
Some of those are: Presearch --> pre_search, Postsearch --> post_search, isempty --> is_empty, exporttojson --> export_tojson etc.

As for Id = arg[2]: the intention of preventing shadowing the reserved id function is understood, but Id is not an option. Adding a trailing underscore for such kind of variable would be acceptable - id_ = arg[2].

Even if you decided to extend a third-party wx.Frame class you should select a consistent way of naming methods in your custom Frame class to not mix method naming style as def search ..., def Display, def addnew and so on.

Conditions

  • is_empty function.
    The condition os.stat(self.jsonfile).st_size == 0 is the same as os.stat(self.jsonfile).st_size (file size can not be less than 0)

  • export_tojson function. The condition:

    if name.GetValue() != '' and age.GetValue() != '' and Id.GetValue() != '':
        if len(data) == 0 and name.GetValue() != '':
    

contains a redundant check name.GetValue() != '' (inner level) as the outer check already ensures it

  • if super().is_empty() is False: is a "noisy" version of if not super().is_empty(): , use the 2nd explicit and short version.

Prefer Multiple Assignment Unpacking Over Indexing
Instead of:

    name = arg[0]
    age = arg[1]
    Id = arg[2]

we use

   name, age, id_ = arg[:3]

Unpacking has less visual noise than accessing the tuple's indexes, and it often requires fewer lines.


Setting values for "found" person:

arg[0].SetValue(f["Name"])
arg[1].SetValue(f["Age"])
arg[2].SetValue(f["Id"])

and for "not found" person:

arg[1].SetValue("---")
arg[2].SetValue("---")
arg[3].SetStatusText("Not Found!")

are repeated across ..._search function and could potentially be extracted to separate functions.

\$\endgroup\$
  • \$\begingroup\$ I updated my post according to your valuable remarks. \$\endgroup\$ – ABA Nov 13 '19 at 10:30

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.