Feedback Request

[tbohon]

I just completed a 'real' application in PB4 and, although it's extremely simple by the standards of most on here, I'm pleased with how quickly it came together.

That said I would like to ask for your thoughts on organization, possible optimization, better ways to do things (like store the data), etc.

And thanks in advance for your constructive criticism ... this is how I best learn new languages, by trying something and then asking the experts for comments ...

Code: Select all

; PureBasic Visual Designer v3.95 build 1485 (PB4Code)


;- Window Constants
;
Enumeration
  #Window_0
EndEnumeration

;- Gadget Constants
;
Enumeration
  #Text_0
  #Text_1
  #Text_2
  #Text_4
  #Text_5
  #Text_6
  #Text_7
  #String_Name
  #String_Address
  #String_City
  #String_State
  #String_ZIP
  #String_Email
  #String_CallSign
  #CheckBox_ARESInfo
  #CheckBox_LicInfo
  #Button_0
  #Button_1
  #Text_Counter
  #Text_Confirm
EndEnumeration

Global Quit.b = #False
Global flength.l = 0


Procedure countlines()
    
  flength = 0
  
  If OpenFile(0, "c:\purebasicwork\safetyfair\efair2.txt")
    While Eof(0)=0
      flength+1
      ReadString(0)
    Wend
    
    CloseFile(0)
    
  EndIf

EndProcedure


Procedure clear_fields()
  
  SetGadgetText(#String_Name, "")
  SetGadgetText(#String_Address, "")
  SetGadgetText(#String_City, "")
  SetGadgetText(#String_State, "WA")
  SetGadgetText(#String_ZIP, "")
  SetGadgetText(#String_Email, "")
  SetGadgetText(#String_CallSign, "")
  SetGadgetState(#CheckBox_ARESInfo, #PB_Checkbox_Unchecked)
  SetGadgetState(#CheckBox_LicInfo, #PB_Checkbox_Unchecked)
  
EndProcedure



Procedure MainProc()
  If OpenWindow(#Window_0, 389, 176, 759, 397, "TC ARES - Emergency Preparedness Expo 2010",  #PB_Window_SystemMenu | #PB_Window_SizeGadget | #PB_Window_TitleBar | #PB_Window_ScreenCentered )
      TextGadget(#Text_0, 120, 60, 100, 20, "Name  ", #PB_Text_Right)
      TextGadget(#Text_1, 110, 90, 110, 20, "Mailing Address  ", #PB_Text_Right)
      TextGadget(#Text_2, 110, 120, 110, 20, "City  ", #PB_Text_Right)
      TextGadget(#Text_4, 360, 120, 50, 20, "State", #PB_Text_Right)
      TextGadget(#Text_5, 500, 120, 70, 20, "Zip Code", #PB_Text_Right)
      TextGadget(#Text_6, 100, 150, 120, 20, "Email  ", #PB_Text_Right)
      TextGadget(#Text_7, 100, 180, 120, 20, "Ham Call sign (if any)  ", #PB_Text_Right)
      StringGadget(#String_Name, 220, 60, 160, 20, "")
      StringGadget(#String_Address, 220, 90, 230, 20, "")
      StringGadget(#String_City, 220, 120, 120, 20, "")
      StringGadget(#String_State, 420, 120, 30, 20, "WA")
      StringGadget(#String_ZIP, 580, 120, 90, 20, "")
      StringGadget(#String_Email, 220, 150, 190, 20, "")
      StringGadget(#String_CallSign, 220, 180, 120, 20, "")
      CheckBoxGadget(#CheckBox_ARESInfo, 220, 220, 260, 20, "Please send me ARES membership information")
      CheckBoxGadget(#CheckBox_LicInfo, 220, 250, 290, 20, "Please send me information on obtaining a ham license")
      ButtonGadget(#Button_0, 350, 310, 80, 40, "Add Me!")
      ButtonGadget(#Button_1, 670, 360, 50, 20, "Exit")
      TextGadget(#Text_Counter, 20, 360, 30, 20, "", #PB_Text_Center)
      TextGadget(#Text_Confirm, 660, 330, 70, 20, "")
      
      SetActiveGadget(#String_Name)
      
      countlines()
      
      SetGadgetText(#Text_Counter, Str(flength))
      
      Repeat
        
        Event.l = WaitWindowEvent()
        
        Select Event
          Case #PB_Event_Gadget
            Select EventGadget()
              Case #Button_0
                
                ares$ = ""
                license$ = ""
                
                name$ = GetGadgetText(#String_Name)
                address$ = GetGadgetText(#String_Address)
                city$ = GetGadgetText(#String_City)
                state$ = GetGadgetText(#String_State)
                zip$ = GetGadgetText(#String_ZIP)
                email$ = GetGadgetText(#String_Email)
                csign$ = GetGadgetText(#String_CallSign)
                
                If GetGadgetState(#CheckBox_ARESInfo) = #PB_Checkbox_Checked
                  ares$ = "ARES YES"
                EndIf
                
                If GetGadgetState(#CheckBox_LicInfo) = #PB_Checkbox_Checked
                  license$ = "LICENSE YES"
                EndIf
                                
                newentry.s = name$ + "|" + address$ + "|" + city$ + "|" + state$ + "|";
                newentry = newentry + zip$ + "|" + email$ + "|" + csign$ + "|"
                newentry = newentry + ares$ + "|" + license$
                
                If OpenFile(0, "c:\purebasicwork\safetyfair\efair2.txt" )
                  FileSeek(0, Lof(0))
                  WriteStringN(0, newentry)
                  CloseFile(0)
                Else
                  MessageRequester("FATAL ERROR!", "Unable to oopen or create output file!!!", #PB_MessageRequester_Ok)
                  Quit = #True
                EndIf
                
                countlines()
                
                SetGadgetText(#Text_Counter, Str(flength))
                
                clear_fields()
                
                SetActiveGadget(#String_Name)
                
              Case #Button_1
                Quit = #True
                
            EndSelect
        EndSelect
        
      Until Event = #PB_Event_CloseWindow Or Quit = #True
      
  EndIf
EndProcedure


MainProc()     ; program entry point

; IDE Options = PureBasic 4.50 (Windows - x86)
; CursorPosition = 60
; FirstLine = 51
; Folding = -
; EnableXP

[Vitor_Boss®]

How you plan read the data?

[c4s]

First tip: Always use EnableExplicit
Second tip: Give the constants a better name! (#Text_0, #Text_1 ...)
Third tip: Use #PB_Any at OpenFile(), OpenImage() etc. or at least a clear constant instead of just 0.
Fourth tip: Try to put actions that come from an event (like EventGadget() -> #Button_0) in to procedures as well.

Everything else comes from learning by doing.

[Fluid Byte]

5.) Get rid of unnecessary blank lines
6.) Use a three char prefix for the constants
7.) Get rid of unnecessary type declarations (.l, .b, etc.). These are completely useless and just slow down the program.
You don't need to assign a type at all because the native type will always be LONG (4 Byte) at x86 and QUAD (8 Byte) on x64.
Only use explicit type declarations for structures to save memory.
8.) Get rid of unnecessary calculations. No need for this

Code: Select all

newentry = newentry + zip$ 

just do it like this

Code: Select all

newentry + zip$ 

9.) No need to set variables to NULL or #False, they are empty by default
10.) I know programmers are lazy but constantly writing everything in lower case is a pain in the ass since it deceases readability
11.) Use the : character to convert a bunch of short single commands to a single line

So this

Code: Select all

Ares$ = ""
License$ = ""

becomes this

Code: Select all

Ares$ = "" : License$ = ""

My final take:

Code: Select all

EnableExplicit

Enumeration
    #WIN_Main
EndEnumeration

Enumeration
    #TXT_Name
    #TXT_Mail
    #TXT_City
    #TXT_State
    #TXT_Zip
    #TXT_Email
    #TXT_Call
    #STR_Name
    #STR_Address
    #STR_City
    #STR_State
    #STR_ZIP
    #STR_Email
    #STR_CallSign
    #CheckBox_ARESInfo
    #CheckBox_LicInfo
    #BTN_Add
    #BTN_Exit
    #Text_Counter
    #Text_Confirm
EndEnumeration

Global Quit, FileLength = 0

Define Event

Procedure countlines()    
    FileLength = 0    
    
    If OpenFile(0, "c:\purebasicwork\safetyfair\efair2.txt")
        While Eof(0) = 0
            FileLength + 1
            ReadString(0)
        Wend
        
        CloseFile(0)        
    EndIf    
EndProcedure

Procedure clear_fields()    
    SetGadgetText(#STR_Name, "")
    SetGadgetText(#STR_Address, "")
    SetGadgetText(#STR_City, "")
    SetGadgetText(#STR_State, "WA")
    SetGadgetText(#STR_ZIP, "")
    SetGadgetText(#STR_Email, "")
    SetGadgetText(#STR_CallSign, "")
    SetGadgetState(#CheckBox_ARESInfo, #PB_Checkbox_Unchecked)
    SetGadgetState(#CheckBox_LicInfo, #PB_Checkbox_Unchecked)    
EndProcedure

Procedure MainProc()
    Protected Event, Ares$, License$, Name$, Address$, City$, State$, Zip$, Email$, CSign$, Newentry.s
    
    If OpenWindow(#WIN_Main, 389, 176, 759, 397, "TC ARES - Emergency Preparedness Expo 2010",  #PB_Window_SystemMenu | #PB_Window_SizeGadget | #PB_Window_TitleBar | #PB_Window_ScreenCentered )
        TextGadget(#TXT_Name, 120, 60, 100, 20, "Name  ", #PB_Text_Right)
        TextGadget(#TXT_Mail, 110, 90, 110, 20, "Mailing Address  ", #PB_Text_Right)
        TextGadget(#TXT_City, 110, 120, 110, 20, "City  ", #PB_Text_Right)
        TextGadget(#TXT_State, 360, 120, 50, 20, "State", #PB_Text_Right)
        TextGadget(#TXT_Zip, 500, 120, 70, 20, "Zip Code", #PB_Text_Right)
        TextGadget(#TXT_Email, 100, 150, 120, 20, "Email  ", #PB_Text_Right)
        TextGadget(#TXT_Call, 100, 180, 120, 20, "Ham Call sign (if any)  ", #PB_Text_Right)
        StringGadget(#STR_Name, 220, 60, 160, 20, "")
        StringGadget(#STR_Address, 220, 90, 230, 20, "")
        StringGadget(#STR_City, 220, 120, 120, 20, "")
        StringGadget(#STR_State, 420, 120, 30, 20, "WA")
        StringGadget(#STR_ZIP, 580, 120, 90, 20, "")
        StringGadget(#STR_Email, 220, 150, 190, 20, "")
        StringGadget(#STR_CallSign, 220, 180, 120, 20, "")
        CheckBoxGadget(#CheckBox_ARESInfo, 220, 220, 260, 20, "Please send me ARES membership information")
        CheckBoxGadget(#CheckBox_LicInfo, 220, 250, 290, 20, "Please send me information on obtaining a ham license")
        ButtonGadget(#BTN_Add, 350, 310, 80, 40, "Add Me!")
        ButtonGadget(#BTN_Exit, 670, 360, 50, 20, "Exit")
        TextGadget(#Text_Counter, 20, 360, 30, 20, "", #PB_Text_Center)
        TextGadget(#Text_Confirm, 660, 330, 70, 20, "")
        
        SetActiveGadget(#STR_Name)
        
        countlines()
        
        SetGadgetText(#Text_Counter, Str(FileLength))
        
        Repeat            
            Event = WaitWindowEvent()
            
            Select Event
                Case #PB_Event_Gadget
                Select EventGadget()
                    Case #BTN_Add                    
                    Ares$ = "" : License$ = ""
                    
                    Name$ = GetGadgetText(#STR_Name)
                    Address$ = GetGadgetText(#STR_Address)
                    City$ = GetGadgetText(#STR_City)
                    State$ = GetGadgetText(#STR_State)
                    Zip$ = GetGadgetText(#STR_ZIP)
                    Email$ = GetGadgetText(#STR_Email)
                    CSign$ = GetGadgetText(#STR_CallSign)
                    
                    If GetGadgetState(#CheckBox_ARESInfo) = #PB_Checkbox_Checked
                        Ares$ = "ARES YES"
                    EndIf
                    
                    If GetGadgetState(#CheckBox_LicInfo) = #PB_Checkbox_Checked
                        License$ = "LICENSE YES"
                    EndIf
                    
                    NewEntry = Name$ + "|" + Address$ + "|" + City$ + "|" + State$ + "|";
                    NewEntry + Zip$ + "|" + Email$ + "|" + CSign$ + "|"
                    NewEntry + Ares$ + "|" + License$
                    
                    If OpenFile(0, "c:\purebasicwork\safetyfair\efair2.txt" )
                        FileSeek(0, Lof(0))
                        WriteStringN(0, newentry)
                        CloseFile(0)
                    Else
                        MessageRequester("FATAL ERROR!", "Unable to oopen or create output file!!!", #PB_MessageRequester_Ok)
                        Quit = #True
                    EndIf
                    
                    countlines()
                    
                    SetGadgetText(#Text_Counter, Str(FileLength))
                    
                    clear_fields()
                    
                    SetActiveGadget(#STR_Name)
                    
                    Case #BTN_Exit
                    Quit = #True                    
                EndSelect
            EndSelect            
        Until Event = #PB_Event_CloseWindow Or Quit = #True        
    EndIf
EndProcedure

[tbohon]

How you plan read the data?

There will be a separate program which we'll use after the Emergency Preparedness Fair has concluded. It will produce a report and be able to produce mailing labels based on various criteria. The only thing this program does is gather the data - we've been doing handwritten rosters for the last few years and, more and more, people can't write or print other than on a keyboard. The contact information we have gotten during those 'handwritten years' has been pretty much useless.

[Trond]

11.) Use the : character to convert a bunch of short single commands to a single line

For me this decreases the readability.

[c4s]

11.) Use the : character to convert a bunch of short single commands to a single line

For me this decreases the readability.

I think for

Code: Select all

a = 1 : b = 0

If a = 1 : b = 2 : EndIf

...it's valid. But yeah, it depends on everyones liking.

[Vera]

Hello,

thanks for the various tips

Though not being an expert I'd like to suggest to use a variable for the file, like e.g.:
Global eFile.s = #PB_Compiler_Home + "efair2.txt" ; -or-
Global eFile.s = #PB_Compiler_FilePath + "efair2.txt"

This would make your code a) crossplattform compatible (what it is right now), b) spare time for any tester to adopt it to local settings and c) makes it much easier for you to handle it on the long run.

Besides this I think it would be good to check if the file is available at program start.

Handling the GUI I would have liked to scroll back&forwards through the datablocks, to apply changes and I missed that ENTER doesn't confirm the entries and set focus to the next one.

greetings ~ Vera