Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Am I doing it fine?? Its only for setup not executed repeatedly

CREATE TABLE #temp
    (
        id               INT IDENTITY(1, 1),
        name_file        VARCHAR(500),
        depth_tree       VARCHAR(10),
        is_folder_files  VARCHAR(10)
    )

    /* xp_dirtree selects file from specific location
    *  depth_tree       :   depth of the search i.e. subfolders
    *  is_folder_files  :   selects folders only or files too
    */

    INSERT INTO #temp(name_file, depth_tree, is_folder_files) EXEC xp_dirtree @source_path, 0, 1

    -- Must concatenate to have permission for xp_cmdshell
    SET @concatenate_string = 'RECONFIGURE EXEC sp_configure ''xp_cmdshell'',1 EXEC MASTER..xp_cmdshell '


    -- Generating copy string in bulk
    SELECT @cmd_string =    
        ISNULL(@cmd_string, '') + 
        CASE WHEN (LEN(REPLACE(t.name_file, @seperate_value, 1)) <> LEN(t.name_file)) -- if @seperate_value is not in image
            THEN
            (
                SELECT CASE 
                    WHEN REPLACE(t.name_file, 'Approach', 1) <> t.name_file OR REPLACE(t.name_file, 'CloseUp', 1) <> t.name_file -- if word Approach or CloseUp is not there in image 
                    THEN
                        (
                        SELECT CASE
                        WHEN ((SELECT f.FaceID FROM Face f WHERE CAST(f.Notes AS VARCHAR) = SUBSTRING(t.name_file, 0, CHARINDEX(@seperate_value, t.name_file)-1)) IS NOT NULL) -- when extracted ID from image <> NotesID
                        THEN
                        (
                        @concatenate_string + '''copy ' + @source_path + t.name_file + ' ' 
                        + @destination_path 
                        + (SELECT f.FaceID FROM Face f WHERE CAST(f.Notes AS VARCHAR) = SUBSTRING(t.name_file, 0, CHARINDEX(@seperate_value, t.name_file)-1)) -- Compares and gives the faceID
                        + (SELECT   CASE 
                                        WHEN REPLACE(t.name_file, 'Approach', 1) <> t.name_file THEN '-AS.jpg'' '
                                        WHEN REPLACE(t.name_file, 'CloseUp', 1) <> t.name_file THEN '-BS.jpg'' '
                                        ELSE
                                            'Undefined'
                                    END
                           )
                        )
                        ELSE
                            ' '
                        END
                    )
                    ELSE
                        ' '
                    END
            )    
            ELSE
                ' '
        END

    FROM #temp t

    SELECT @cmd_string + 'RECONFIGURE EXEC sp_configure ''xp_cmdshell'',0'

    EXEC (@cmd_string)
share|improve this question

1 Answer 1

I think it might be useful to separate comments about the code itself from comments about your general approach (here is your related question on SO for reference).

In general the code is fine, but there are some small points that you could consider:

  • 'concatenate' is a verb, not a noun, so @concatenate_string is a slightly awkward name for a variable; it would make more sense for a function or stored procedure.
  • 'seperate' is not correctly spelled, it should be 'separate'. I understand that English may not be your first language but it is extremely difficult to search code effectively if words are misspelled. In practice I know this isn't easy to check - because you may not even know you're making a mistake - but as a general suggestion, if you ever have a doubt about the spelling of a word while you're typing, you should take the time to check it.
  • Code should not rely on undocumented language or platform features (xp_dirtree in this case) because they may change or disappear at any time
  • You haven't included your variable declarations so I don't know what data type @cmd_string is, but you have declared @concatenate_string as varchar so I guess you may have done the same for @cmd_string. In this case nvarchar would be better: not only is it the correct data type for use with xp_cmdshell but it will avoid any issues with Unicode data. Note that when using nvarchar all literals should be preceded with N: N'literal'
  • A PRINT @cmd_string statement would be useful for debugging

As for your general approach:

  • I understood from your SO question that this is a one-time script that will not be re-used, therefore you consider that using undocumented features (xp_dirtree) and features that by default only sysadmins can use (xp_cmdshell) is reasonable. In my experience, the idea that a one-time script can be written more 'casually' than other code is doubtful: 'one-time' scripts often turn out to be re-used or adapted for other purposes, and even genuinely one-time tasks are good practice for 'doing it right'. However everyone's situation is different, so this is only a general observation and you may have very good reasons for using xp_cmdshell
  • SQL Server is not a good tool for working with files. If I was going to use your approach, I would probably generate only the copy commands using SQL, save them as a .cmd file and run them outside the database. This would avoid messing with xp_cmdshell completely
  • xp_cmdshell can occasionally be useful but in general it has too many issues. In particular, since it executes under the SQL Server service account (for sysadmins) you may end up having to give that account permissions that it shouldn't have in order to make your script run. If you then forget to revoke the permissions afterwards, your service account can accumulate unnecessary permissions over time, to the point where it becomes a real risk
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.