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()
-
\$\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– greybeard2019年11月12日 21:33:02 +00:00Commented Nov 12, 2019 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\$Hob_io– Hob_io2019年11月13日 10:34:02 +00:00Commented Nov 13, 2019 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– Heslacher2019年11月13日 10:38:19 +00:00Commented Nov 13, 2019 at 10:38
1 Answer 1
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 conditionos.stat(self.jsonfile).st_size == 0
is the same asos.stat(self.jsonfile).st_size
(file size can not be less than0
)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 ofif 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.
-
\$\begingroup\$ I updated my post according to your valuable remarks. \$\endgroup\$Hob_io– Hob_io2019年11月13日 10:30:16 +00:00Commented Nov 13, 2019 at 10:30