View Issue Details

IDProjectCategoryView StatusLast Update
12074Development Otherpublic2017-04-25 11:48
ReporterjiripavlicekAssigned To 
PrioritynoneSeverityfeature 
Status newResolutionopen 
Product Version2.5x 
Target VersionFixed in Version 
Summary12074: code readability/manageability improvement
Description

Hi all,
thank you for your good work on Linesurvey! I have small improvement for better code readability/manageability - there is used so much strings like 'A', 'B', 'C', ... in source in conditions for question types. I replaces those string by constants. Why? Modern IDE's can show you where constant/variable/class/whatever else is used in source, but searching for string like 'A' can't return the same comfortable result. I made it fo question types, but there are other string for graph types and in import/export. may be next time I'll improve other source code part this way.
Yes, I know, that best of all would be to create general question type class and inherit it for all question types. But this change will be hard to make, so I made only first step - constants.
I'll create pull request in a while.
Let's discuss it.

Steps To Reproduce

This isn't any bug/issue report. This is created as announce of my pull request for improve code readability/quality. It doesn't fixes any bug/issue or add new feature. This is only code development improvement.

TagsNo tags attached.

Activities

DenisChenu

DenisChenu

2017-01-23 08:58

developer   ~42791

Really like the idea :) Maybe review contant name.

Example :
SINGLE_CHOICE_5POINT
SINGLE_CHOICE_LANGUAGE
SINGLE_CHOICE_LIST_RADIO
SINGLE_CHOICE_LIST_DROPDOWN
etc ...
TEXT_SHORT
TEXT_LONG
TEXT_HUGE

The idea : 1st part is ~ DB type, after put detail.

jiripavlicek

jiripavlicek

2017-01-25 08:19

reporter   ~42825

Hi DenisChenu,

where is the best place to contact you? Here in issues tracker or in pull request discussion https://github.com/LimeSurvey/LimeSurvey/pull/629 ?

I created constant names like QT_A_ARRAY_5_CHOICE_QUESTIONS mainly to help me check my replacements ;) Character "A" is here for easier checking to replaced right string by right constant. Also a left the same constant values - for example QT_A_ARRAY_5_CHOICE_QUESTIONS has value "A", main reason for that was not to broke Limesurvey in case I forget somewhere replace "A" string.

Let me know your idea about Question class constants names and I'll rename it and make new pull request.

DenisChenu

DenisChenu

2017-01-25 09:02

developer   ~42828

@jiripavlicek : yes pull request seems best, i'm not the only dev :).

@olle think it's best to have the old code (like you do)
@c-schmitz think it's best to make the pull for develop.

jiripavlicek

jiripavlicek

2017-02-09 08:05

reporter   ~42988

Any ideas how should I change my pull request to be suitable for you? Thanks.

LouisGac

LouisGac

2017-04-25 11:48

manager   ~43440

your PR is a great work. If you want to port it yourself, just do it in the develop branch, it will be released with LS3 in few months (with Twig engine for templates).

Issue History

Date Modified Username Field Change
2017-01-23 08:38 jiripavlicek New Issue
2017-01-23 08:52 DenisChenu Severity minor => feature
2017-01-23 08:58 DenisChenu Note Added: 42791
2017-01-25 08:19 jiripavlicek Note Added: 42825
2017-01-25 09:02 DenisChenu Note Added: 42828
2017-02-09 08:05 jiripavlicek Note Added: 42988
2017-04-25 11:48 LouisGac Note Added: 43440