[Last Call] Learn how to a build a cloud-first strategyRegister Now

x
  • Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 876
  • Last Modified:

Can this code improved ?

Hello,

Following the discussions of july and august and want to start a new topic.
Prior related question:
http://www.experts-exchange.com/Q_26321089.html

I used the valuelisteditor for an application to add a location.
Going on and with help of experts i improved the code.

Two open issues:
1. The problem was that u must must use many booleans to make it work comform specifications.
2. I recognized that the setfocus method  not always select the field (make green on my computer)
    How can I make the error field selectable also when the cursor is on an other control ? (it happens  when   the focus is on button1)

Other comments are also welcome.

Here is code sofar ( I changed it a little )


procedure TForm1.ValueListEditor1SelectCell(Sender: TObject; ACol,
   ARow: Integer; var CanSelect: Boolean);
   begin
     if valid = false  then
     begin
       if ((ACol = ErrorCol) and (Arow = ErrorRow)) or (closing) then
          canselect:= true
       else
          canselect := false;

       button1.enabled:=false;
     end
     else
     begin
       CanSelect := true;
       button1.enabled:= true
     end;

     if closing then
        button1.enabled:=true
   end;



procedure TForm1.ValueListEditor1StringsChange(Sender: TObject);
begin
// check only as strings change and not close
   if not Closing
     then
       Checked:=False
     else
       Checked:=True;

end;


Function SetErrorCaption (ACol:Integer;ARow:Integer):string;
var Caption:String;
begin
   Form1.ValueListEditor1.Col:= ACol;
   Form11.ValueListEditor1.Row:= ARow;

   ErrorCol:= ACol;
   ErrorRow:= ARow;

   if Acol = 0 then
       Caption:= 'Locatie name not valid';

   if Acol = 1 then
       Caption:= 'number of places not valid';


   Agendaform1.Valuelisteditor1.Setfocus;

   result:= Caption;
end;

procedure TForm1.ValueListEditor1Validate(Sender: TObject; ACol, 
ARow: Integer;
const KeyName, KeyValue:string);

var KeyValueInt,PreviousValue:Integer; ErrorCaption:string;
begin

Try
if Checked= false then
begin

    Checked:=True;

    if trim (Keyname) = '' then
       begin
         ErrorCaption:= SetErrorCaption(0,Arow);
         raise Exception.create ('Locatiename has no valid value') 
       end;

     if tryStrtoInt (Trim(KeyValue),KeyValueInt) then
     begin
       if (KeyValueInt < 0) or (KeyvalueInt > 999)  then
         begin
           ErrorCaption:= SetErrorCaption(1,Arow);
           raise Exception.create('errormessage2');
         end
   
   
end;//checked
    // on exception show messagebox
    except
      on E : Exception do
       begin
         Windows.MessageBox(0,pWideChar(E.Message),PwideChar(ErrorCaption),mb_Ok);
         valid:=false;
       end;
    end; //except

end; //procedure

Open in new window

0
advance1
Asked:
advance1
  • 5
  • 4
3 Solutions
 
epasquierCommented:
1) If I remember your problem correctly, I think that your current code is about how tight as you can have it. You need those booleans to track some events combinations/order. So...

You could change a few details in coding style, but mostly there would be no change in the compiled results, only more readability of the code. I'm talking about details like
   If SomeBoolean = False Then
instead of
   If Not SomeBoolean Then

or the use of Windows.MessageBox instead of ShowMessage Delphi function.

or the fact that you can use Result as any variable, you don't need to set a value to a local variable and at the end set the result:=ThatVariable. Use directly Result.

and that :
   if not Closing
     then
       Checked:=False
     else
       Checked:=True;
can be replaced by :
  Checked:=Closing; // Much simpler no ?

2) I'm not sure what you are talking about with "I recognized that the setfocus method  not always select the field (make green on my computer)"
- If you mean that what you need is that the grid get the focus, that is what SetFocus is for.
- if you want also that a certain cell is selected, and so the focus shows a frame around the cell, that is what setting row/col is for
- and last, if by "make green" you mean that the text of this cell is selected in edit mode, the best you can do is to set the EditorMode property to True after the SetFocus

Why do you have Form1 and AgendaForm1 in your function ? is it a typo or do you really have 2 forms ? If you have only one, then you should consider making this function a method of that form, that would remove the need of specifying the form and use directly the ValueListEditor control.

If you really have 2 forms, please explain...
Function SetErrorCaption (ACol:Integer;ARow:Integer):string;
begin
// Form1.ValueListEditor1.Col:= ACol;
// Form1.ValueListEditor1.Row:= ARow;

 ErrorCol:= ACol;
 ErrorRow:= ARow;

 if Acol = 0 then result:= 'Locatie name not valid';
 if Acol = 1 then result:= 'number of places not valid';

 With Agendaform1.Valuelisteditor1 do
  begin
   Col:= ACol;
   Row:= ARow;
   Setfocus;
   EditorMode:=True;
  end;
end;

Open in new window

0
 
epasquierCommented:
thanks for the move of the code, it was almost unreadable before
0
 
advance1Author Commented:

Thks for your comments  Good that you noticed,
I changed AgendaForm1 to Form1. So There is only 1 form (Form1).

I will check your changes.
0
Independent Software Vendors: We Want Your Opinion

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

 
Ephraim WangoyaCommented:
Just a few things I can think of on top of what epasquire has already noted

procedure TForm1.ValueListEditor1SelectCell(Sender: TObject; ACol,
   ARow: Integer; var CanSelect: Boolean);
begin
     if valid = false  then
     begin
         canselect := closing or ((ACol = ErrorCol) and (Arow = ErrorRow)) ;
         button1.enabled:=false;
     end
     else
      ........  
 end;

since you only execute these statements if Checked is false, you can put your try after the check
procedure TForm1.ValueListEditor1Validate(Sender: TObject; ACol, ARow: Integer;
  const KeyName, KeyValue:string);
var
  KeyValueInt, PreviousValue: Integer;
  ErrorCaption:string;
begin
  if Checked = false then
    try
      Checked := True;

      if Trim (Keyname) = '' then
      begin
        ErrorCaption:= SetErrorCaption(0,Arow);
        raise Exception.create ('Locatiename has no valid value')
      end;

      if tryStrtoInt (Trim(KeyValue), KeyValueInt) then
      begin
        if (KeyValueInt < 0) or (KeyvalueInt > 999)  then
         begin
           ErrorCaption:= SetErrorCaption(1,Arow);
           raise Exception.create('errormessage2');
         end
      end;//checked
      // on exception show messagebox
    except
      on E : Exception do
       begin
         Windows.MessageBox(0,pWideChar(E.Message),PwideChar(ErrorCaption),mb_Ok);
         valid:=false;
       end;
    end; //except
end; //procedure

You can also have
 case ACol of
    0: Result := 'Locatie name not valid';
    1: Result := 'number of places not valid';
  end;
0
 
advance1Author Commented:
Thks for the comments.
I think the comments are correct.
Its the old discussion is more cryptic code better code. I dont think this always so,

About the selectmode  I want the text of this cell is selected in edit mode,
I set the EditorMode property to True after the SetFocus, but when I clicked on button1 the error field is still not selected (This happens when I click the mouse).


0
 
epasquierCommented:
you mean it's in edit mode, but not selected ?

I'm not sure that can be done (easily). I'll check that tomorrow if you confirm that is what you want  
0
 
advance1Author Commented:
Yes, I mean that.
Thks in advance !
0
 
epasquierCommented:
Well, as I said it's not 'doable' by conventional weapons. But a mini-nuke might help to get it done quite quickly.
The idea is to use the InplaceEditor property of any custom grid (from which TValueListEditor is derived), but it is a protected property, so unatainable.
What we have to do is make that property available, and that is done by redefining it
type
// redefinition of the TValueListEditor class
  TValueListEditor = class(ValEdit.TValueListEditor)
  public
    property InplaceEditor; // change this property to public
  end;
  TForm1 = class(TForm)
// ... rest of your form definition


//in the code, now you can access the InplaceEditor property
 With Agendaform1.Valuelisteditor1 do
  begin
   Col:= ACol;
   Row:= ARow;
   Setfocus;
   EditorMode:=True;
   InplaceEditor.SelectAll; // hehe!
  end;

Open in new window

0
 
advance1Author Commented:
Sounds good , I will try it Monday
0
 
advance1Author Commented:
Yess its ok !!

A lot of code but it works !

Thks.
I keep de question few days  open for other reactions,
0

Featured Post

What does it mean to be "Always On"?

Is your cloud always on? With an Always On cloud you won't have to worry about downtime for maintenance or software application code updates, ensuring that your bottom line isn't affected.

  • 5
  • 4
Tackle projects and never again get stuck behind a technical roadblock.
Join Now