Ajax Class Efficiency So I wrote this class to handle my ajax calls simply:

ajaxCon.php:

class ajaxCon{
    private $init = false;
    private $functionArray = array();
    private $Message = false;
    private $function = null;
    private $arguments = null;
    private $post = null;

    public function __construct(){
        if(!isset($_POST['ajaxRequest'], $_POST['ajaxFunction'])) return;
        $this->init = true;
        ob_start();
        $this->post = $this->sanitizeArr($_POST);
        $this->arguments = $this->sanitizeArr($_POST['arguments']);
        $this->function = $_POST['ajaxFunction'];
    }

    public function addFunction($functionName, $functionCode){
        if(!$this->init || !isset($functionName, $functionCode) || $functionName != $this->function) return $this;
        $this->Message = call_user_func_array($functionCode, $this->arguments) ? 'success' : 'fail' ;
        unset($this->init, $this->functionArray, $this->post);
        $returnVars = get_object_vars($this);
        ob_clean();
        echo json_encode($returnVars);
        ob_flush();
        exit();
    }

    private function sanitizeArr($returnArr, $sanitizer = 'sanitize_sql_string') {
        array_walk_recursive($returnArr, function (&$value) use ($sanitizer) {
            $value = $sanitizer($value);
        });

        return $returnArr;
    }
}

ajaxCon.js:

function ajax() {

    this.callback = false;

    this.ajaxFailure = function (returnData) {};

    this.ajaxSuccess = function (returnData) {};

    this.call = function(functionName, argumentArray, callback){
        $.ajax( $.extend(true, {}, this.ajaxSettings, {data:{'ajaxFunction':functionName, 'arguments':argumentArray}} ) );
        this.callback = callback;
    };

    this.ajaxReturn = function (returnData){
        returnData = returnData.substring(returnData.indexOf("{") - 1);
        returnData = returnData.substring(0, returnData.lastIndexOf("}") + 1);
        var responseArray = $.parseJSON(returnData);

        if (responseArray.Message.toLowerCase() != "success"){
            this.ajaxFailure(responseArray);
            return;
        }

        this.ajaxSuccess(responseArray);
        if(this.callback != false) this.callback(responseArray);
    };

    window.ajaxCon = this;
    this.ajaxSettings = { url: window.location.href, type: 'POST', cache: false, data: {ajaxRequest: true}, success: function (returnData) {
        ajaxCon.ajaxReturn(returnData)
    }};
}

and you call it like so:

<?php
    $ajaxCon = new ajaxCon;

    $ajaxCon->addFunction('checkEmail', function($Email){
        global $ajaxCon;
        if(emailIsInDB($Email)) $ajaxCon->hasAccount = true;
        return true;
    });
?>
<script type="text/javascript">
    var ajaxCon = new ajaxCon();

    function checkEmail() {
        ajaxCon.call('checkEmail', {'Email':$('#Email').val()}, function(result){
            if(!result.hasAccount) $('#SignUp').submit();
        });
    }
</script>

The drawbacks I know of are lot's of preprocessing (The entire page has to be preprocesed) and the page then executes as normal until the php function I need is defined and called.

Any ideas on how I can optimize the code and make it run more efficiently while preserving/improving readability?

A review of the JavaScript part:

ajaxCon is a class/constructor, so you should call it AjaxCon, though really I would think about finding an even better name :)
ajaxFailure -> You have a number of functions that have ajax in their name, since these functions are in the Ajax constructor you can drop the ajax part
this.callback = false; -> Don't declare callback at all and then use if(this.callback) this.callback(responseArray);
This could use some newlines:
$.ajax( $.extend(true, {}, this.ajaxSettings, {data:{'ajaxFunction':functionName, 'arguments':argumentArray}} ) );

and also

this.ajaxSettings = { url: window.location.href, type: 'POST', cache: false, data: {ajaxRequest: true}, success: function (returnData) {
    ajaxCon.ajaxReturn(returnData)
}};

Also the location of this.ajaxSettings is odd, this belongs on top of that function


Function to discard repeats, applied to numbers Solving a 4Clojure problem, I got this answer to compress a sequence (problem 30):

#(map first (partition-by identity %))

What would be the best approach to have this work with plain numbers too?

My solution was this:

(defmulti comp class)
(defmethod comp java.lang.Long [x] x)
(defmethod comp :default [x] (map first (partition-by identity %)))

The code is more than tripled.
Are there better solutions? Shorter? More idiomatic?
I tried extending java.lang.Long with ISeqable, but failed miserably.

Nice solution. Your solution already works great with sequences of numbers (and more) already. The comp multi-method is not a good idea. It is a function which can take either a seq or a number. You should avoid creating functions that take arguments that can be different types (it is confusing and suggests your might be better off with 2 functions or a protocol) unless there is a really good reason. In this case comp does not add anything useful (why did you add it? do you have an example use case?)


Jasmine unit test that triggers mouse events in Angular 4 I have the following jasmine test spec which triggers the mouse events in my directive. As you can see there is a lot of code duplication here, I am repeating the events here.

describe('Test for my module', () => {
        let fixture: ComponentFixture<TestComponent>;
        let de: DebugElement[];
        let service: MyService = new MyService();

        beforeEach(() => {
            TestBed.configureTestingModule({
                declarations: [Move, TestComponent],
                providers: [MyService]
            });
            fixture = TestBed.createComponent(TestComponent);
            service = fixture.debugElement.injector.get(MyService);
            fixture.detectChanges();
            de = fixture.debugElement.queryAll(By.directive(Move));
        });

// My test spec. This is where I need advice for removing the code duplication.
it('test the move event', inject([MyService], (service: MyService) => {
                let object = spyOn(service.x, 'emit');
                spyObj.and.callThrough();
                de[0].triggerEventHandler('mousedown', { pageX: 10, pageY: 10 });
                de[0].triggerEventHandler('mousemove', { pageX: 40, pageY: 20 });
                expect(service.x.emit).toHaveBeenCalledWith({ x: 30, y: 10 });
                object.calls.reset();
                de[0].triggerEventHandler('mousemove', { pageX: 45, pageY: 25 });
                expect(service.x.emit).toHaveBeenCalledWith({ x: 35, y: 15 });
                object.calls.reset();
                de[0].triggerEventHandler('mousemove', { pageX: 50, pageY: 30 });
                expect(service.x.emit).toHaveBeenCalledWith({ x: 40, y: 20 });

            }));

Could anyone help me by suggesting any way to avoid this duplication?

I did come up with one alternative which is as follows:

    function triggerEvents(debugElement: DebugElement, eventName: string, object: any){
            debugElement.triggerEventHandler(eventName, object);
        }

    function expectAndReset(spy: Function, object: any, spyObj: jasmine.Spy) {
        expect(spy).toHaveBeenCalledWith(object);
        spyObj.calls.reset();
    }

    it('test the move event', inject([MyService], (service: MyService) => {
        let spyObj = spyOn(service.x, 'emit');
        spyObj.and.callThrough();
        triggerEvents(de[0],'mousedown', { pageX: 10, pageY: 10 });
        triggerEvents(de[0],'mousemove', { pageX: 40, pageY: 20 });
        expectAndReset(service.x.emit,{ x: 30, y: 10 },spyObj);
        triggerEvents(de[0],'mousemove', { pageX: 45, pageY: 25 });
        expectAndReset(service.x.emit,{ x: 35, y: 15 },spyObj);
        triggerEvents(de[0],'mousemove', { pageX: 50, pageY: 30 });
        expectAndReset(service.x.emit,{ x: 40, y: 20 },spyObj);

    }));

I am not really satisfied with my own solution though, because the function implementation of triggerEvents is almost identical as the triggerEventHandler in the Angular Documentation. I am not doing anything special there. But I am hoping that the code reviewers on the community would comment or share their views and give me a better solution :)

Second Attempt:

        function triggerEvents(debugElement: DebugElement, eventName: string, object: any) {
            debugElement.triggerEventHandler(eventName, object);
        }

        function expectAndReset(spy: Function, object: any, spyObj: jasmine.Spy) {
            expect(spy).toHaveBeenCalledWith(object);
            spyObj.calls.reset();
        }

        it('test the move event', inject([MyService], (service: MyService) => {
            let spyObj = spyOn(service.x, 'emit');
            spyObj.and.callThrough();
            triggerEvents(de[0], 'mousedown', { pageX: 10, pageY: 10 });
            for (let i = 0; i <= 10; i += 5) {
                triggerEvents(de[0], 'mousemove', { pageX: 40 + i, pageY: 20 + i });
                expectAndReset(service.x.emit, { x: 30 + i, y: 10 + i }, spyObj);
            }

        }));

Somewhat satisfied with my second iteration.

Thank you.


Data Access Layer Code Wondering if I could do this a little bit better. Currently, my DAL has a blank constructor that sets the connection string from the web.config.

private string cnnString;

public DAL()
{
    cnnString = ConfigurationManager.ConnectionStrings["ApplicationServices"].ConnectionString;
}

Then each method in here is mapped directly to a stored procedure, and they ALL look something like this:

public bool spInsertFeedback(string name, string subject, string message)
{
    int rows = 0;

    SqlConnection connection = new SqlConnection(cnnString);
    try
    {
        connection.Open();
        SqlCommand command = new SqlCommand("[dbo].[spInsertFeedback]", connection);
        command.CommandType = CommandType.StoredProcedure;

        // params
        SqlParameter messageName = new SqlParameter("@name", SqlDbType.VarChar);
        messageName.Value = name;
        SqlParameter messageSubject = new SqlParameter("@subject", SqlDbType.VarChar);
        messageSubject.Value = subject;
        SqlParameter messageText = new SqlParameter("@message", SqlDbType.VarChar);
        messageText.Value = message;

        // add params
        command.Parameters.Add(messageName);
        command.Parameters.Add(messageSubject);
        command.Parameters.Add(messageText);

        rows = command.ExecuteNonQuery();
    }
    catch
    {
        return (rows != 0);
    }
    finally
    {
        connection.Close();
    }

    return (rows != 0);
}

Obviously some return a DataSet or a list of an object or something, where as this one just returns whether or not any rows were affected.

However, each method does things this way, and I just feel like I have a lot of redundant code, and I'd like to simplify it. From the other classes, I'm calling the DAL like this:

DAL dal = new DAL();
bool success = dal.spInsertFeedback(name, subject, message);
return Json(success);

Thanks guys.

I would not recommend putting your dal code in a try-catch block. At least you should be logging or doing something about it, currently you're just returning false (or no value). If an exception occur, let it happen, or put a try-catch on the function that calls it. Remember that exceptions vary (did the db server stop? was the sql malformed? these all are different cases.)

Using using is a better practice for all IDisposable objects, thus embrace it. If you run Visual Studio's code analysis tools (Ultimate Edition only I guess) it would also tell you to do so.

My version of your code is similar to Jeff's (which I wanted to vote up but didn't have enough rep yet) with an exception, SqlCommand is also disposable and can/should be used with using block.

public bool spInsertFeedback(string name, string subject, string message)
 {
     int rows = 0;
     using (var connection = new SqlConnection(cnnString))
     {
         connection.Open();
         using (var command = new SqlCommand("[dbo].[spInsertFeedback]", connection))
         {
             command.CommandType = CommandType.StoredProcedure;
             // params
             SqlParameter messageName = new SqlParameter("@name", SqlDbType.VarChar);
             messageName.Value = name;
             SqlParameter messageSubject = new SqlParameter("@subject", SqlDbType.VarChar);
             messageSubject.Value = subject;
             SqlParameter messageText = new SqlParameter("@message", SqlDbType.VarChar);
             messageText.Value = message;

             // add params
             command.Parameters.Add(messageName);
             command.Parameters.Add(messageSubject);
             command.Parameters.Add(messageText);
             rows = command.ExecuteNonQuery();
         }
     }
     return (rows != 0);
 }

Also I would suggest you to use connectionString instead of cnnString, which is not a proper naming.


Helper function to find rotation of Caesar Cipher Just finished an assignment in Python where one part was to write a function to find the correct rotation in a encryption similar to a Caesar Cipher..

As mentioned it's already finished and works as intended. The course is an introduction to Python, but I do have experience with other languages since previously.

I was hoping someone could have a look at my findRot to see where I possibly could have done anything that isn't 'Python-esque' or what potential bottlenecks might occur.

def findRot(word1, word2, iterator):
    letterFreq = [("A", 65), ("E", 69), ("I", 73), ("O", 79), ("T", 84), ("U", 85)]
    rot = ord(word1) - letterFreq[iterator][1]
    letters = [chr(ord(letter) - rot) if (ord(letter) - rot >= 65) else chr(90 - (65 -(ord(word2[2]) - rot + 1))) for letter in word2]
    with open('words.txt', 'r') as words:
        lines = words.readlines()
        lines = list(map(lambda s: s.upper().strip(), lines))
        if ''.join(letters) in lines:
            print(''.join(letters) + ", " + str(True))
            return rot
        else:
            findRot(word1, word2, iterator + 1)

The assignment specified the usage of letter frequency, so I just took some of the more common letters found in English and put them in a list of tuples for readability purposes.

In the professor's use-case the first word would be 'R' and the second would be 'Uxen'. So in this case the rotation would supply the 'I' and 'Love'.

Again, this function just runs a primary check to find the rotation to apply to the rest of the 'encrypted' text. So a validation of the decrypted text will take place later.

EDIT:

Example usage:

>>> findRot('R', 'UXEN', 0)
9

Because it takes 9 rotations to get to the clear text I and Love.

First off, you should follow PEP8.
This will make your code more readable for Python programmers, and there is quite a few tips in there to make your code more Pythonic.
The amount of whitespace that you use is actually pretty good! It's mostly your names that 'let you down'.
But you're following a convention of camelCase. Which is also ok, but snake_case is preferred.

I don't like the re-definition of letterFreq and lines on ever function call.
This leads to a performance hindrance.
And so I'd move them out of the function to make them global.
Also as they are constants you may want to follow PEP8 and name them LETTER_FREQ and WORDS.

After this I'd look into removing the recursion. In Python recursion is cute, and un-reliable, nothing more.
I'd recommend that you don't use it, and instead you can use a for loop.
Using range starting at iterator and ending at len(LETTER_FREQ) we can remove the need for the recursion.

I'd also remove the print, instead of calling it in the function you should do it outside of the function.
If I'm using this function I don't want a print there ever couple of minutes.

Finally your turnery is a bit awkward and so I'd change it to use the modulo function, %.
This allows you to remove the if/else as -1 % 26 == 25 and so we can replace it with chr((ord(letter) - rot - 65) % 26 + 65).

Adding all the above together lead me to:

LETTER_FREQ = "AEIOTU"

with open('words.txt', 'r') as f:
    WORDS = [word.upper().strip() for word in f.readlines()]

def find_rotation(word1, word2, iterator=0):
    for index in range(iterator, len(LETTER_FREQ)):
        rot = ord(word1) - ord(LETTER_FREQ[index])
        letters = ''.join([chr((ord(letter) - rot - 65) % 26 + 65) for letter in word2])
        if letters in WORDS:
            return rot
    return None

For an additional speed you can also change WORDS to be a set, WORDS = set(WORDS), this is as it'll then be \$O(1)\$ look up in the if letters in WORDS: line, rather than \$O(n)\$. This is good as if the words list is large then it'll take a significantly larger amount of time then it would with a set.


HttpClient wrapper for simple calls with optional cert Trying to ensure each controller contains it's own httpclient.  The super user can change which endpoint is called from the site and some of those endpoints do not require a certificate, but some require their own certificate.

Want a safe call to this wrapper class (HttpClientProvider) as a static and in a static constructor for reuse. Any gotchas?

Will the httpclient still execute after not being used for a while?

Here is the Controller Call:

    public class HomeController : Controller
{
    private static readonly HttpClientProvider _clientProvider;

    static HomeController()
    {
        _clientProvider = new HttpClientProvider();
    }

    public ActionResult Index()
    {
        var certPath = HostingEnvironment.MapPath("~/App_Data/cert.pfx");
        var httpClient = _clientProvider.GetHttpClient(certPath);

        var result = httpClient.GetAsync("www.example.com").Result;

        return View();
    }
}

Here is the Provider Class:

 public class HttpClientProvider
{
    private HttpClient _client;
    private WebRequestHandler _clientHandler;

    public HttpClient GetHttpClient()
    {
        SetUpHandler();
        SetUpClient();

        return _client;
    }

    public HttpClient GetHttpClient(string certPath, string password = "")
    {
        SetUpHandler();
        SetUpClient();
        SetUpCertificate(certPath, password);

        return _client;
    }

    public HttpClient GetHttpClient(X509Certificate2 x509Cert)
    {
        SetUpHandler();

        _clientHandler.ClientCertificates.Add(x509Cert);

        SetUpClient();

        return _client;
    }

    private void SetUpHandler()
    {
        if (_clientHandler == null)
        {
            _clientHandler = new WebRequestHandler();
        }
        else
        {
            _clientHandler.ClientCertificates.Clear();
        }
    }

    private void SetUpCertificate(string certPath, string password = "")
    {
        if (certPath != null)
        {
            var absolutePath = HostingEnvironment.MapPath(certPath);

            var cert = absolutePath != null
                ? new X509Certificate2(absolutePath, password, X509KeyStorageFlags.MachineKeySet)
                : new X509Certificate2(certPath, password, X509KeyStorageFlags.MachineKeySet);

            _clientHandler.ClientCertificates.Add(cert);
        }
    }

    private void SetUpClient()
    {
        if (_client == null)
        {
            _client = new HttpClient(_clientHandler);
        }
    }
}

As is this does not work because multiple requests will concurrently operate on the same variables (instance variables and HttpClient internal state as it is being configured).

This is fairly easy to solve because there's a nice pattern for this. Your core problem is that you need to initialize one expensive object for each instance of a certain kind of key (here: HttpClient and certificate path).

I'm modifying some existing code to ensure that only one HttpClient per key is ever created:

public static class HttpClientManager
{
    private static ConcurrentDictionary<string, Lazy<HttpClient>> _clients =
        new ConcurrentDictionary<string, Lazy<HttpClient>>();

    public static HttpClient Get(string certificatePath)
    {
        return _clients.GetOrAdd(certificatePath, _ =>
            new Lazy<HttpClient>(CreateClient(certificatePath));
    }

    static HttpClient CreateClient(string certificatePath)
    {
        //TODO
    }
}

You only need to fill in the TODO.

This code is thread safe because the infrastructure takes care of only invoking your factory method once per unique certificatePath. The factory will only use local state so that it is totally unconcerned with thread safety.

I'd avoid methods such as void SetUpXxx(...). These messods make use of side-effects for no reason. In particular, _clientHandler seems to be used only as a way to thread this value through the various methods being called. It is often better to avoid passing data in such covert ways. Convert that field to a method parameter and explicitly pass that value around. I'd do the same thing for _client itself. Then, you can keep the SetUpXxx methods but they become static. It will be explicit what data they consume and what they output.

This might seems like a subtle point but I found this kind of change helpful over and over again.


Retry storing HTTP response into a variable until specific code I want to keep trying to get response until its code is 200 or unknown yet. In first case it should be stored in response variable. In another case I should raise any kind of exception.

  response = nil
  1.times do
    response = begin
      http.request request
    rescue Net::ReadTimeout
      puts "Net::ReadTimeout"
      retry
    end
    case response.code
    when "503"
      puts "servers are busy at #{Time.now}?"
      sleep 5
      redo
    when "200"
      "ok"
    else
      fail "#{response.code} at '#{request.path}'"
    end
  end

The 1.times thing is taken from SO.

The 1.times ... redo thing is just awful. Replace that with an until loop, like this:

success = false
until success
  ...
  case response.code
  ...
  when "200"
    success = true
    "ok"
  else
    ...
  end
end


Memory management for matrices built with dynamic arrays I am working on C++ code which builds matrices (3D and 4D) using dynamic arrays.
I am using arrays instead of std::vector; as in the future, I might extend this code to some kind of multithreading or parallel process (e.g. CUDA).

In my code, I often initialize and define the source matrix in main.cpp.

Then I have a function (sometimes it may belong to a class) which inputs the matrix along with other variable and I need to return the modified matrix. Which is subsequently used in main.

This is what I usually work with in my code:

    #include <iostream>
    #include <stdio.h>
    #include <stdlib.h>
    using namespace std;

    float ***matFunc ( ***mat3D, int n);

    int main()
    {
      // define dimensions of the matrix
      int M , N, S = 3;

     //define and initialize the matrix
      float ***myMat = new float **[M];
         for (int i = 0; i< M; i++ ) {
              myMat[i] = new float *[N];
              for ( int j = 0; j < N; j++ ) {
                 myMat[i][j] = new float [S];
                    for (int k = 0; k < S; k++ ) {
                      myMat[i][j[k] = someValue obtained or calculated
                          }
                   }
              }

         // here I call my member function to perform addition
         // This is also used if I have more than one matrices

        float ***newMat = matFunc( myMat, int n ); //returns modified matrix

         //deleting the original myMat

         for (int i = 0; i < M; i++) {
             for (int j = 0; j < N; j++) {
               delete[]myMat[i][j];
        }
            delete[] myMat[i];
      }
   delete[]myMat;

      return 0;
     }

     //function definition for matFunc

      float***matFunc(float ***mat, int n ){

       int p, q, r;
       int M, N, S = 3;

           float ***modifiedMat = new float **[M];
                for ( p = 0, p < M; p++ ) {
                   modifiedMat[p] = new float *[N];
                     for ( q = 0; q < n; q++ ) {
                         modifiedmat[p][q] = new float [S];
                          for (r  =0; r < S; r++ ) {
                          modifiedMat[p][q][r] = mat[p][q][r] * do something
                         }
                        }
                      }

           return modifiedMat;
        }

As you can observe, I am always defining and initializing new matrices with help of dynamic arrays. This becomes more troublesome if matrix dimensions are to be increased. I use dynamic arrays due to their faster operations as compared to vectors. But I think there might be some memory management issues with code.  As I have observed it consumes more RAM and sometimes my IDE/compiler will complain lack of index or space for integer type.

Could you please suggest how I can improve on this code?

At first, you should give your program some structure.

Start by indenting all the lines consistently. Ideally, let a program do this automatically. Search for indenter or beautifier.

Then, define a class called Matrix, give it a proper constructor and destructor. This could already help with the memory leak.

Or if you don't like classes, at least define an allocation and a deallocation function for a matrix. But then, it's almost a class anyway.

Finally, fix the syntax errors in your code so that it compiles at least.

Make the code in main so simple to read that you can explain it to a layman. That is, no explicit memory allocation, no details, just the high level view. All the rest of the code needs to go to separate functions.


Text file searching using SQL I have the need to search a directory containing 1000+ files on remote servers where I only have access to SQL Server Management Studio and Explorer. I have written the below SQL statement which does what I need, only incredibly slowly. I fear using this many temp tables and the cursor is the main issue. Any ideas on how I can speed this up?

EDIT: I forgot to mention that I need to keep this query based.  I will not necessarily have full permissions regarding jobs and/or SSIS packages.

--Drop temp table if it already exists
IF OBJECT_ID('tempdb..#tempFileContents') IS NOT NULL
DROP TABLE #tempFileContents

DECLARE @FindText AS VARCHAR(255)
DECLARE @FileDir AS VARCHAR(255)

--Directory to search
SET @FileDir = 'C:\Users\*********\Desktop\TEMP\'
--Text to search
SET @FindText = '1234'
----------------------
----------------------

--Crete temp table to store output lines from text file
CREATE TABLE #tempFileContents
(
    lineText TEXT
)

--Create declared table to store found file names in the directory
DECLARE @files table
(
    filename VARCHAR(255)
    ,depth INTEGER
    ,files BIT
)

--Create declared table to store the text the string ws found in along with the filpath              of the file.
DECLARE @foundText table
(
    LineNum INT
    ,foundText TEXT
    ,filename VARCHAR(255)
)

--Create table of files in the directory
INSERT INTO @files
EXEC xp_dirtree @FileDir, 10, 1

--Update file names to include filepath
UPDATE @files SET filename = @FileDir + filename

--Varchar used to store the derived SQL string
DECLARE @sql NVARCHAR(1000);

--Create cursor for file searching
DECLARE @text VARCHAR(255)
DECLARE @ID VARCHAR(255)
DECLARE IDs CURSOR LOCAL FOR select filename from @files

--Open cursor
OPEN IDs
FETCH NEXT FROM IDs into @ID
WHILE @@FETCH_STATUS = 0
BEGIN
    --Truncate temp table prior to searching a file
    TRUNCATE TABLE #tempFileContents
    SET @sql = 'BULK INSERT #tempFileContents FROM "' + @ID + '"'
    EXEC(@sql);

    --Insert into results table
    INSERT INTO @foundText
    (LineNum, foundText, filename)
    SELECT ROW_NUMBER() OVER( ORDER BY @ID ) AS 'rownumber', lineText, @ID FROM #tempFileContents

    DELETE FROM @foundText
    WHERE foundText NOT LIKE '%' + @FindText + '%'
    OR foundText IS NULL

    FETCH NEXT FROM IDs into @ID
END

--Close off and dispose of unneeded resources
CLOSE IDs
DEALLOCATE IDs
DROP TABLE #tempFileContents

--Display results
SELECT * FROM @foundText

Although the approach is 'peculiar in choice of tools', doing what you do should work just fine. The only part that I would definitely want to change is the loading of the results. Currently you are BULK INSERTing a file into a temp-table, then copy all that data into a table-variable, followed by deleting everything from the freshly (and the old!) copied data where it does not match a given criteria.

I would propose the following changes:

do not use Table-variables for this. They're fine for (very) small data-sets that don't require a lot of manipulation but will confuse the Query Optimizer when you do lots of inserts and deletes on them
do not use the TEXT datatype, rather use varchar(max)
use IDENITITY instead of RowNumber(). RownNumber works great, but it requires sorting the data. Also be very careful with this: doing RowNumber() ordered over a 'constant' might return different results than you originally anticipated and most certainly does not guarantee that you'll get things in the order they were originally in the file !!!!!
variable naming: @filename is much more readable than @ID

Doing some cleaning I ended up with below which in my tests on a couple of 100 small files ran slightly faster than the original. There might be a nicer way than the tempdb..stagingTable approach which will cause issues when the script is run in parallel, but I didn't see an easier way out to be able to use LineNum as an identity which in my opinion is must for what you're trying to achieve!

--Drop temp table if it already exists
IF OBJECT_ID('tempdb..tempdb..stagingTable') IS NOT NULL
    DROP TABLE tempdb..stagingTable

IF OBJECT_ID('tempdb..#files') IS NOT NULL
    DROP TABLE #files

IF OBJECT_ID('tempdb..#foundText') IS NOT NULL
    DROP TABLE #foundText

----------------------
----------------------
GO
--Crete temp table to store output lines from text file
CREATE TABLE tempdb..stagingTable
(
    LineNum int IDENTITY(1, 1) PRIMARY KEY,
    LineText varchar(max),
)

GO
-- since we can't tell BULK-insert to skip the LineNum column, we create a view without it
CREATE VIEW vtempFileContents AS SELECT LineText FROM tempdb..stagingTable
GO

--Create declared table to store found file names in the directory
CREATE TABLE #files
(
    filename VARCHAR(255)
    ,depth INTEGER
    ,files BIT
)

--Create declared table to store the text the string ws found in along with the filpath of the file.
CREATE TABLE #foundText
(
    LineNum int
    ,foundText TEXT
    ,filename VARCHAR(255),
    PRIMARY KEY (filename, LineNum)
)

GO

DECLARE @FindText AS VARCHAR(255)
DECLARE @FileDir AS VARCHAR(255)

--Directory to search
SET @FileDir = 'C:\temp\test\'
--Text to search
SET @FindText = 'common'

--Create table of files in the directory
INSERT INTO #files
EXEC xp_dirtree @FileDir, 10, 1

--Varchar used to store the derived SQL string
DECLARE @sql NVARCHAR(1000);

--Create cursor for file searching
DECLARE @text VARCHAR(255)
DECLARE @filename VARCHAR(255)

DECLARE files_loop CURSOR LOCAL FAST_FORWARD FOR SELECT @FileDir + filename FROM #files

--Open cursor
OPEN files_loop
FETCH NEXT FROM files_loop into @filename
WHILE @@FETCH_STATUS = 0
BEGIN
    --Truncate temp table prior to searching a file
    TRUNCATE TABLE tempdb..stagingTable
    SET @sql = 'BULK INSERT vtempFileContents FROM "' + @filename + '"'
    EXEC(@sql);

    --Insert matching records in results table
    INSERT INTO #foundText (LineNum, foundText, filename)
    SELECT LineNum, LineText, @filename
      FROM tempdb..stagingTable
     WHERE LineText IS NOT NULL
       AND LineText LIKE '%' + @FindText + '%'

    FETCH NEXT FROM files_loop into @filename
END

--Close off and dispose of unneeded resources
CLOSE files_loop
DEALLOCATE files_loop

--Display results
SELECT * FROM #foundText

GO
-- cleanup
DROP VIEW vtempFileContents
DROP TABLE tempdb..stagingTable
DROP TABLE #foundText
DROP TABLE #files


React App with external route table and cookie-based routing This is a small frontend frame for a React app that is built then served statically.
Because the app uses React-Router for browser-based routing but the server doesn't know anything about the app I added some code to the backend that reads the list of routes the app uses from a json file and creates dummy handlers for each that place a redirect=/path cookie and redirect to the main page. That code is left out for brevity and to focus on the frontend.
Meanwhile, the Javascript React frontend in this question sets ups the app's React-Router routes based on the same external json file. The main page checks whether there is a redirect cookie set by the server and if so sends the user to that page.
The overall effect between the frontend and backend is that a user can go directly to app.com/widget/4 and without them knowing the server bounces them to the main page to start the browser-routing session and then back to the correct url
routes.json:
{
  "routes": [
    {"path":"/widget/:widgetId", "component": "WidgetContainer", "redirect": true},
    {"path":"/", "component": "MainPageContainer", "exact": true}
  ]
}

index.js:
import React from 'react';
import ReactDOM from 'react-dom';
import { BrowserRouter as Router, Route } from "react-router-dom";

import  ParseRoutes from "./Utils/ParseRoutes"
import { routes } from "./routes.json"

function AppContainer() {
  const routeProps = ParseRoutes(routes)

  return (
    <Router>
     {routeProps.map((props, i) => (<Route key={i} {...props} /> ))}
    </Router>
  )
}

ReactDOM.render(<AppContainer />, document.getElementById("root"))

ParseRoutes.js:
import  MainPageContainer from "../Components/MainPage"
import  WidgetContainer from "../Components/Widget"

const stringToComp = {
    MainPageContainer: MainPageContainer,
    WidgetContainer: WidgetContainer
}

const ParseRoutes = (routes) => (
    routes.map((props) => ({...props, component: stringToComp[props.component]}))
)

export default ParseRoutes

RedirectIfRequested.js:
import React from "react"
import { Redirect } from "react-router-dom"

const getCookieValue = (a) => {
    var b = document.cookie.match('(^|;)\\s*' + a + '\\s*=\\s*([^;]+)')
    return b ? b.pop() : ''
  }

const RedirectIfRequested = () => {
    const redirectTo = getCookieValue("redirect")

    if ( redirectTo ) {
        document.cookie = "redirect=''; max-age=-1"
        return <Redirect to={redirectTo} />
    }

    return null
}

export default RedirectIfRequested

MainPage.js:
import React from "react"

import RedirectIfRequested from "../Utils/RedirectIfRequested.js"

export default function MainPageContainer(props) {
    const redirect = RedirectIfRequested()

    return redirect ? redirect : <MainPage />
}

function MainPage(props) {
    return
Main Page

}

Widget.js:
import React from "react"

import { useParams } from "react-router-dom"

export default function WidgetContainer(props) {
    const id = useParams().widgetId

    return
 Widget {id} accessed via react-router

}

In general, your code does a good job at naming variables/functions, and at splitting logic up in ways that make sense.
However, there are a few places where you are inconsistent with your code style - maybe using a linter would help out:

In some files you indent with 2 spaces. Other files are indented with 4.
There's a number of import statements that have an extra space between the words. e.g. import  ParseRoutes from "./Utils/ParseRoutes"

A note on casing: Generally, we use TitleCase for classes and components only. camelCase is used for almost everything else. A file (or folder with an index.js) with a default export of a class or component may use TitleCase too. However, folders like "Util" or "Components" should probably all be in lowercase. Your "getCookieValue()" function is properly cased, but in the same file, your "RedirectIfRequested()" function is not - RedirectIfRequested is not a class or component. (there's a couple other examples like that).
The local variables in getCookieValue() could probably be named better then "a" and "b". Maybe rename "a" to "cookieKey" and "b" to "match".
.pop() is not a common way to get a group from a regex. If you want to get the second group, just say that with code: match[2]. Note that since you don't actually care about the value of the first group in your regex, you can use a non-capturing group instead (the syntax is (?:...)). For example, if your regex is a(b|B)c(d|D)e, then b/B is group 1 and d/D is group 2, but if its a(?:b|B)c(d|D)e, then you can't get b/B, and d/D is group 1. You don't have to use non-capture groups, but I just want to make sure you know that that's an option.

Finally, and most importantly, all of this routing logic you made is actually not needed. The proper way to configure a server to handle single-page-application (SPA) routing is as follows:
Lets assume your SPA exits at example.com/my-app, and your app has routes such as example.com/my-app/ (the home page) and example.com/my-app/widget/2. You need to configure your server to return the same react bundle no matter which route they hit, as long as its within example.com/my-app. So, going to example.com/my-app/does-not-exist will give you the same javascript bundle as going to example.com/my-app/widget/2. (Note that this is not the same as a redirection). Finally, javascript can check which URL you used (was it /my-app/does-not-exist/ or /my-app/widget/2) and route based on that information - something that react-router will do for you. If the route doesn't exist, just show a 404 page from your React app.


Call html() on two jQuery selector objects In my webpage I have two places where I need my JavaScript to write dates. How to do it in one go?

Selectors:

var $today = $('#today');
var $selected = $('#selected');

If I were to do this alternatively the easy way, I'd do:

$('#today, #selected').html(date);

But I kind of need to reuse my #selected jQuery object throughout my code.

The only solution I came up with so far is...

$([$today, $selected]).each(function(idx, obj){obj.html(date);});

Is there a better way?

I believe you would be better off combining the two jQuery objects using .add() rather than a comma in the selector.  It's also more elegant than having to call .each().

$today.add($selected).html(date);


Iterative sum using recursion Given the following recursive definition of sum:

(define (sum term a next b)
  (if (> a b)
      0
      (+ (term a)
         (sum term (next a) next b))))

And the task:

  Exercise 1.30

  The sum procedure
  above generates a linear recursion.
  The procedure can be rewritten so that
  the sum is performed iteratively. Show
  how to do this by filling in the
  missing expressions in the following
  definition:

(define (sum term a next b)
  (define (iter a result)
    (if <??>
        <??>
        (iter <??> <??>)))
  (iter <??> <??>))

I wrote the following code.  What do you think?

(define (i-sum term a next b)
  (define (iter a result)
    (if (> a b)
        result
        (iter (next a) (+ result (term a)))))
  (iter a 0))

(define (identity x) x)
(define (inc x) (+ 1 x))
(define (sum-integers a b) (i-sum identity a inc b))

I believe your answer is correct, although I'm not sure why you need the identity, inc, and sum-integers procedure on the bottom for your solution.


Check that file is fully downloaded by FTP I have a task in which I want to handle error occurred in FTP downloading, For which I used the wget command to download a file via FTP and once the download is complete, compare downloaded filesize with FTP file size.

For that, I have the code as follows. My question: is this the correct method?

# fetch file details from the server and extract only number from the return value

FileSizeOnServer=curl --max-time 10 -u $FTP_USER:$FTP_PASSWORD --head $FTP_Path 2>&1 | grep "Content-Length:"
echo "Present File Size Details:" $FileSizeOnServer

FTP_File_Size=${FileSizeOnServer#*: }
echo "FTP_File_Size:$FTP_File_Size"

# fetch only size of local file size

DownloadedFileSize=`stat -c %s /home/pi/Desktop/ABCD.txt`
echo "DownloadedFileSize:$DownloadedFileSize"

# compare local and ftp file size

if [ "$DownloadedFileSize" -eq "$FTP_File_Size" ]; then
    echo "Size matched"
else
    echo "File Size doesn't match Start Downloading again"
fi

You definitely need more quoting of parameter expansions.  Notably, $FTP_PASSWORD may well contain characters that are meaningful to the shell:

FileSizeOnServer=$(curl --max-time 10 -u "$FTP_USER:$FTP_PASSWORD" \
                   --head "$FTP_Path" 2>&1 | grep "Content-Length:")

I'm not sure it's a good idea to redirect standard error there - you want errors to be visible, not piped into grep.

I'm not sure there's much value to the size comparison, though - curl should exit with non-zero status if it didn't transfer the whole file, so we should just use it directly:

if ! curl -u "$FTP_USER:$FTP_PASSWORD" "$FTP_Path"
then
    echo "Failed to download" >&2
    exit 1
fi

Or even simply

set -e
curl -u "$FTP_USER:$FTP_PASSWORD" "$FTP_Path"


Async - await all the way Related to this question (and even this Stack Overflow's one) I have been trying to avoid blocking the code. So what I understood it's that is preferable to make all the methods that rely on an async one async themselve (thus not defeating the purpose of asynchronous). Therefore I have changed my code to do the following (I include here more "layers" than in the questions I linked as I had to change accordingly those).

My API, exposing a method:

[Route("api/postconfiguration")]
public async Task<IHttpActionResult> PostConfiguration(configurationData configurationData)
{
    try
    {
        await _configurationRequestHandler.HandlePostRequest(configurationData);
        return Ok();
    }
    catch (Exception e)
    {
        // Log error
        return InternalServerError(e);
    }
}

That uses this method:

public async Task HandlePostRequest(Data Data)
{
    var configuration = await _ConfigurationService.GetConfigurationAsync(Data.AgentId);

    // Use that configuration
}

And this would be the method calling the external api

public class ConfigurationApiClient : IConfigurationService
{
    private readonly HttpClient _client;
    private const string resourcePath = "configuration/{0}";

    public ConfigurationApiClient(IConfigurationManager configurationManager)
    {
        _client = new HttpClient();
        _client.BaseAddress = new Uri(configurationManager.GetAppSetting("ConfigurationApiUrl"));
        _client.DefaultRequestHeaders.Accept.Clear();
        _client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
    }

    public async Task<Configuration> GetConfigurationAsync(int agentId)
    {
        try
        {
            var response = await _client.GetAsync(String.Format(resourcePath, agentId));
            response.EnsureSuccessStatusCode();
            return await response.Content.ReadAsAsync<Configuration>();
        }
        catch (HttpRequestException ex)
        {
            throw new Exception(string.Format("Exception when getting  configuration for agent: {0}", agentId), ex);
        }
    }

}

This is my understanding of what I should do for making it asynchronous, but would appreciate if someone points me any error.

I am using the API on one client myself, which does this:

public async Task DeployConfiguration(Package package, EFEnvironment environment)
{
    using (var client = new HttpClient())
    {
        // Prepare the configurationData

        var agentTasks = agents.Select(a =>
            {
                var configurationPostRequest = new ConfigurationData
                    {
                        // Create it
                    };

                return client.PostAsJsonAsync("api/postconfiguration", configurationPostRequest);
            });

        await Task.WhenAll(agentTasks);
    }
}

Which is just called inside a sync method:

_deployApiService.DeployConfiguration(package, environment);

Your code falls into a classic trap, which IMO is a design error by Microsoft. 99% of the time, await Foo(); should be await Foo().ConfigureAwait(false);. The semi-official guidance is

  To summarize this third guideline, you should use ConfigureAwait when possible. Context-free code has better performance for GUI applications and is a useful technique for avoiding deadlocks when working with a partially async codebase. The exceptions to this guideline are methods that require the context.

I think that it would have been far better for ConfigureAwait(false) to be the default and to require ConfigureAwait(true) in those cases which require context, but it's too late now for Microsoft to change this, so we should all get into the habit of using ConfigureAwait(false) by default and in those rare cases documenting the reason for leaving it off (or even explicitly using ConfigureAwait(true) and supplementing it with a comment which gives the reason).


Update on tileMap Class This is an update from An Update on my TileMap Class. It's a tileMap class for a top-down old-school final fantasy game. User @Tim gave me suggestions and I updated some of the code accordingly.

Tile.java

import com.stardust.rpgtest2.gfx.Assets;
import java.awt.image.BufferedImage;

public enum Tile
{
    Grass(1, Assets.grass, false),
    Dirt(2, Assets.dirt, false),
    Tree(3, Assets.tree, true),
    Blank(36, Assets.blankTile, false);

    public int texKey;
    public BufferedImage texture;
    public boolean collidable;
    Tile(int texKey, BufferedImage texture, boolean collidable)
    {
       this.texKey = texKey;
       this.texture = texture;
       this.collidable = collidable;
    }
    public static BufferedImage getTileImage(int texKey1)
    {
        BufferedImage image;
        switch(texKey1)
        {
            case 1:
            {
                image = Grass.texture;
                break;
            }
            case 2:
            {
                image = Dirt.texture;
                break;
            }
            case 3:
            {
                image = Tree.texture;
                break;
            }

            case 36:
            {
                image = Blank.texture;
                break;
            }
            default:
            {
                image = Blank.texture;
                break;
            }
        }
        return image;
    }
}

TileMap.java render() method

public void render(Graphics g)
    {

        for(int y = 0; y < tilemap.length; y++)
        {
            for(int x = 0; x < tilemap.length; x++)
            {
                int textureType = tilemap[x][y];

                BufferedImage texture =  Tile.getTileImage(textureType);
                g.drawImage(texture, posX, posY, null);
                posY += 32;
            }
            posX += 32;
            posY = Game.mapY;
        }
        posX = Game.mapX;
        posY = Game.mapY;
    }

posY += 32;
        }
        posX += 32;

You have your tile sizes hardcoded in the TileMap ... in an obscure place (render code).

Consider placing them as constants in Tile or TileMap. That way, upscaling to 64x64 tiles is a lot easier.

public static BufferedImage getTileImage(int texKey1)
{
    BufferedImage image;
    switch(texKey1)
    {
        case 1:
        {
            image = Grass.texture;
            break;
        }
        case 2:
        {
            image = Dirt.texture;
            break;
        }
        case 3:
        {
            image = Tree.texture;
            break;
        }

        case 36:
        {
            image = Blank.texture;
            break;
        }
        default:
        {
            image = Blank.texture;
            break;
        }
    }
    return image;
}

You could use a Map here. Map<Integer, BufferedImage>, or better, Map<Integer,Tile>. Then you just have to get the Tile from the map, and ask that for the image. And if it's null you can return a blank.


Extracting distinct HTML list items into a JavaScript array This code is made to generate a list of unique elements of the array made of the other two arrays.
The fact that we need two lists to convert in two different array is part of the task.
This code handles with the task, but there's a lot of global variables. As far as I know, it is not good practice.
Tell me, please, how can I improve it?

Любовь

Надежда

Вера

София

Надежда

Вера

<button id="makeNewArray">Создать новый массив</button>

As already noticed by previous comments, don't mind about global variables if they're declared by var inside of a function.
And you can do the same for commonArray and str, unless they're used elsewhere (obviously, I don't think so).

In the other hand, you can dramatically reduce your code!

First because you don't have to wait for onload event, since your

Here is a working example.


Combining these Add and Remove methods into 1 I was wondering if it's possible to move these add remove methods into a single method:

public void AddToVipGroup(int companyID, string userID, string auditTrailUrl)
{
    string userName = "", groupName = "";
    GetUserAndGroupNames(companyID, userID, out userName, out groupName);
    GroupUtilities.AddUserToGroup(userName, groupName, auditTrailUrl);
}

public void RemoveFromVipGroup(int companyID, string userID, string auditTrailUrl)
{
    string userName = "", groupName = "";
    GetUserAndGroupNames(companyID, userID, out userName, out groupName);
    GroupUtilities.RemoveUserFromGroup(userName, groupName, auditTrailUrl);
}

private void GetUserAndGroupNames(int companyID, string userID, out string userName, out string groupName)
{
    var vipGroupSID = new MyDataAccess().GetVipGroupSID(companyID);
    if (string.IsNullOrEmpty(vipGroupSID))
        throw new Exception(string.Format("Company id: {0} has no vips group in db", companyID));

    //add user in Company Access vips group
    DBUser user = DBUser.LoadBySid(userID);
    DBGroup group = DBGroup.LoadBySid(vipGroupSID);

    userName = user.UserName;
    groupName = group.Name;
}

I am only looking to merge these methods, if it would be considered best practice.

It isn't a good idea to merge these methods. They do completely opposite things - you never want a method like:

public void DoSomething(bool arbitraryFlag)
{
    if (arbitraryFlag)
    {
        DoSomething1();
    }
    else
    {
        DoSomethingCompletelyDifferent();
    }
}

I will expand on what paritosh commented though.

private void GetUserAndGroupNames(int companyID, string userID, out string userName, out string groupName)

This signature smells because it returns void but has 2 out parameters.

A better signature would be like:

public class UserAndGroup
{
    public string Username { get; set; }
    public string GroupName { get; set; }
}

public UserAndGroup GetUserAndGroup(int companyId, string userId)
{
    var vipGroupSID = new MyDataAccess().GetVipGroupSID(companyID);
    if (string.IsNullOrEmpty(vipGroupSID))
    {
        // see edit
        throw new Exception(string.Format("Company id: {0} has no vips group in db", companyID));
    }

    var user = DBUser.LoadBySid(userID);
    var group = DBGroup.LoadBySid(vipGroupSID);

    return new UserAndGroup
    {
        Username = user.UserName,
        GroupName = group.Name
    };
}

Note that I've introduced var where they type is obvious, added braces around the if statements and removed the very confusing comment that was there.

Your other methods would then become something like:

public void RemoveFromVipGroup(int companyID, string userID, string auditTrailUrl)
{
    var userAndGroup = GetUserAndGroup(companyID, userID);
    GroupUtilities.RemoveUserFromGroup(userAndGroup.Username, userAndGroup.GroupName, auditTrailUrl);
}

I still think it's a bit weird though... Is there only one group? It's odd to add/remove a user to a group without being able to specify what that group is.

EDIT

You should never throw System.Exception - there is always a better, specific exception to throw. If there isn't, create one (as a last resort).

For example, you could create a GroupMissingException or something that makes sense for your domain.


Small Counter by typescript with React I am learning typescript with React and come up with this small program that each button count how many time by clicking and record which button got click. Please help me to take a look if all the code are the best practice.
import * as React from "react";
import "./styles.css";

interface ButtonContainerProps {
  options: string[];
}
interface ButtonProps {
  value: string;
  setSelectedOption(value: string): any;
}

const Button: React.FunctionComponent<ButtonProps> = (props): JSX.Element => {
  const { value, setSelectedOption } = props;
  const [count, setCount] = React.useState(0);
  const clickHandler = React.useCallback(() => {
    setCount(count + 1);
    setSelectedOption(value);
  }, [count, value, setSelectedOption]);
  return <button onClick={clickHandler}>{`${value}: ${count}`}</button>;
};

const Scoreboard: React.FunctionComponent<{
  selectedOption: null | string;
}> = ({ selectedOption }): JSX.Element => {
  return (

      {selectedOption
        ? `The selected option is ${selectedOption}`
        : `Please click following button to make an option`}

  );
};

const ButtonContainer: React.FunctionComponent<ButtonContainerProps> = ({
  options
}): JSX.Element => {
  const [selectedOption, setSelectedOption] = React.useState<string | null>(
    null
  );
  return (

      <Scoreboard selectedOption={selectedOption} />
      {options.map((option) => (
        <Button
          key={option}
          value={option}
          setSelectedOption={setSelectedOption}
        />
      ))}

  );
};
export default function App() {
  return (

Button Container

      <ButtonContainer options={["apple", "orange", "pear"]} />

  );
}

React.FunctionComponent probably isn't necessary - it's boilerplate for every single component you have, and arguably makes typings a bit worse, not better. See this article. For example, you could accidentally do:
<Scoreboard selectedOption={selectedOption}>{'The Scoreboard'}</Scoreboard>

without an error being thrown because every React.FunctionComponent is typed as taking children, even if they're not used.
To make the types more accurate, and to reduce the amount of boilerplate, consider using completely plain functions instead.
Declare variables close to where they'll be used The ButtonContainerProps is declared at the top of the script, but it isn't used until you scroll past 2 intermediate components - so if you see
const ButtonContainer: React.FunctionComponent<ButtonContainerProps> = ({

and you don't remember what ButtonContainerProps is, you'll have to scroll all the way back up to the top to see what it is. Or, even better:
If something is only going to be used once, consider whether it can be reasonably defined inline. For example:
const ButtonContainer: ({ options }: { options: string[] }) => {

I think that's plenty clear.
Let TS automatically infer return types when possible - unless explicitly typing a value is necessary to make the TS compile, or to make it easier for a reader to understand, you consider just leaving it off, like with the above ButtonContainer, since the boilerplate doesn't accomplish much. The capitalization of a component should make it clear that the function is a component that returns JSX.
Possible duplicate key It's probably pretty unlikely, but if the caller of ButtonContainer passes an array of strings that contains a duplicate, the <Button key={option} will result in a duplicate key, which is a problem. If the buttons are completely static in the DOM, I'd make the key be the index of the element in the options:
{options.map((option, i) => (
  <Button
    key={i}

Or filter out duplicates from the options before rendering.
value name? One might normally expect a variable named value to contain something from the user that might change, like something in an input box, or a count of something. Here, the value being passed to Button is actually a name or label connected to the button (and the use of value alongside a stateful count variable might seem odd). Consider renaming it to name or buttonLabel or something of the sort. (Probably not option, since it's not related to <option>)
useCallback is useful when optimizing performance and reducing unnecessary re-renders when passing the callback as a prop to a child component. But here, there's just a <button> (not a child component) which'll only change on click (won't update frequently enough for the overhead of a function call to matter at all). The useCallback is introducing complication without accomplishing anything useful here; I'd consider removing it.


Hanging semicolons in multiline statements I'm writing code to do line printing to a template -- counting characters to box A, then to box B, etc. -- and as such, it's not very modular. There are a lot of (long) variable names and each statement, which adds to the string to be printed, is pretty long.

So I've been breaking up the statements across multiple lines. But I don't know how I should be indenting it or where I should put the semicolon.

Here's a sample line:

$rbt .= "\n"
    . str_repeat(" ", $rbtSt_billToBox_leadingWidth)
    . padBothAndTruncate($statementInfoArray['bill-to-addr1'], $rbtSt_billToBox_addr_maxWidth)
    . str_repeat(" ", $rbtSt_billToBox_innerMargin)
    . substr($statementInfoArray['bill-to-code'], 0, $rbtSt_billToBox_acctNum_maxWidth)
;

I like how the concat operators are lined up and how it looks bracketed, but the naked semicolon on the following line makes me a little nervous. Putting it at the end of the line makes me even more nervous, since then it's hard to see.

It gets a little trickier when there's more nesting.

$rbt .= "\n"
    . str_repeat(" ", $rbtSt_billToBox_leadingWidth)
    . padBothAndTruncate(
        $statementInfoArray['bill-to-city'] . ", " . $statementInfoArray['bill-to-state']
            . " " . $statementInfoArray['bill-to-zip']
    )
;

Do I always do multiple lines? Just when it is more than a certain number of characters (say, 120)? Once a statement is multiline, should I only have one operator per line?

I'm feeling a bit paralyzed by choices here and I fear that none of them are very good.

This is indeed purely personal, however, when you work in a group, or want to share your code, it's best to stick to certain conventions. A good source for this is:

http://www.php-fig.org/psr/psr-2

Eventhough these guidelines are quite good, I don't always follow them myself. For instance, I often use two spaces to indent, not four. Etc.

As for my personal opinion about any code: It should be very easy to read and understand. Your code wouldn't be accepted here, mainly due to the lack of comments and sometimes vague names. But that's not what your question is about. As for the concatenation dots, I like them at the end of lines:

$startOfLine = PHP_EOL.
               str_repeat(' ',$rbtSt_billToBox_leadingWidth);
$innerMargin = str_repeat(' ',$rbtSt_billToBox_innerMargin);
extract($statementInfoArray,'stm');
$rbt .= $startOfLine.
        padBothAndTruncate($stm_bill-to-addr1,$rbtSt_billToBox_addr_maxWidth).
        $innerMargin.
        substr($stm_bill-to-code,0,$rbtSt_billToBox_acctNum_maxWidth).
        $startOfLine.
        padBothAndTruncate($stm_bill-to-city.', '.$stm_bill-to-state.' '.$stm_bill-to-zip);

and then I align all the parts on the left side. This makes it very clear what belongs together. I am however sure there are many other ways to do this.


Rock Paper Scissors Lizard Spock in JQuery I wrote a little Javascript rock/paper/scissors/lizard/spock game and when the user clicks the button to make their choice from the five options, I choose the computer's randomly  and then compare the two to find a winner and output the result. It works, but I have the feeling that the findWinner function can be a lot shorter or more elegant. I'm trying to improve my Javascript and jQuery and was hoping for someone to help me with next steps here:.

function findWinner(choiceA,choiceB) {
  if (choiceA == choiceB) {
    return "It's a tie!";
  }

  if (choiceA == 'Rock') {
    if (choiceB == 'Lizard' || choiceB == 'Scissors') {
      return "You win!";
    }
    else if (choiceB == 'Spock' || choiceB == 'Paper') {
      return choiceB;
    }
  }
  else if (choiceA == 'Paper') {
    if (choiceB == 'Rock' || choiceB == 'Spock') {
      return "You win!";
    }
    else if (choiceB == 'Scissors' || choiceB == 'Lizard') {
      return "Computer wins :( ";
    }
  }
  else if (choiceA == 'Scissors') {
    if (choiceB == 'Paper' || choiceB == 'Lizard') {
      return "You win!";
    }
    else if (choiceB == 'Spock' || choiceB == 'Rock') {
      return "Computer wins :( ";
    }
  }
  else if (choiceA == 'Lizard') {
    if (choiceB == 'Paper' || choiceB == 'Spock') {
      return "You win!";
    }
    else if (choiceB == 'Rock' || choiceB == 'Scissors') {
      return "Computer wins :( ";
    }
  }
  else if (choiceA == 'Spock') {
    if (choiceB == 'Rock' || choiceB == 'Scissor') {
      return "You win!";
    }
    else if (choiceB == 'Paper' || choiceB == 'Lizard') {
      return "Computer wins :( ";
    }
  }
}

This just seems very repetitive, but I'm not sure where to go next with it.

Write down the rules in an object. If an object contains a key, it means it can beat it.

Also, you're writing a javascript game, not jQuery. jQuery is a javascript library.

var rules = {
  rock: {
    scissors: true,
    lizard: true
  },
  scissors: {
    paper: true,
    lizard: true
  },
  lizard: {
    paper: true,
    spock: true
  },
  spock: {
    rock: true,
    scissors: true
  },
  paper: {
    rock: true,
    spock: true
  }
};

function play(opt1, opt2) {
  if (opt1 === opt2) {
    return "Draw";
  }

  if (rules[opt1][opt2]) {
    return "You win";
  }

  return "You loose";
}

console.log(play("rock", "scissors"));
console.log(play("paper", "paper"));
console.log(play("lizard", "spock"));


Python optimized base64 writer for streamed files I needed to make a base64 file encoder where you can control the read buffer size. This is what I came up with and it's quite fast. It might be able to be simpler but still maintain its performance characteristics. Any suggestions?

def chunked_base64_encode(input, input_size, output, read_size=1024):
    """
    Read a file in configurable sized chunks and write to it base64
    encoded to an output file.

    This is an optimization over ``base64.encode`` which only reads 57
    bytes at a time from the input file. Normally this is OK if the
    file in question is opened with ``open`` because Python will
    actually read the data into a larger buffer and only feed out
    57 bytes at a time. But if the input file is something like a
    file stream that's read over the network, only 57 bytes will be
    read at a time. This is very slow if the file stream is not
    buffered some other way.

    This is the case for MongoDB GridFS. The GridOut file returned by
    GridFS is not a normal file on disk. Instead it's a file read in
    256 KB chunks from MongoDB. If you read from it 57 bytes at a time,
    GridFS will read 256 KB then make lots of copies of that chunk
    to return only 57 bytes at a time. By reading in chunks equal
    to the GridFS chunk size, performance is 300 times better.

    Performance comparison:

        File size 10 MB
        Save to MongoDB took 0.271495819092 seconds
        Fast Base 64 encode (chunk size 261120) took 0.250380992889 seconds
        Base 64 encode (chunk size 57) took 62.9280769825 seconds

        File size 100 MB
        Save to MongoDB took 0.994009971619 seconds
        Fast Base 64 encode (chunk size 261120) took 2.78231501579 seconds
        Base 64 encode (chunk size 57) took 645.734956026 seconds

    For regular files on disk, there is no noticeable performance gain
    for this function over ``base64.encode`` because of Python's built
    in buffering for disk files.

    Args:
        input (file): File like object (implements ``read()``).
        input_size (int): Size of file in bytes
        output (file): File like object (implements ``write()``).
        read_size (int): How many bytes to read from ``input`` at
            a time
    """
    # 57 bytes of input will be 76 bytes of base64
    chunk_size = base64.MAXBINSIZE
    base64_line_size = base64.MAXLINESIZE
    # Read size needs to be in increments of chunk size for base64
    # output to be RFC 3548 compliant.
    read_size = read_size - (read_size % chunk_size)
    num_reads = int(ceil(input_size / float(read_size)))
    # RFC 3548 says lines should be 76 chars
    base64_lines_per_read = read_size / chunk_size

    input.seek(0)
    for r in xrange(num_reads):
        is_last_read = r == num_reads - 1
        s = input.read(read_size)
        if not s:
            # If this were to happen, then ``input_size`` is wrong or
            # the file is corrupt.
            raise ValueError(
                u'Expected to need to read %d times but got no data back on read %d' % (
                    num_reads, r + 1))

        data = b2a_base64(s)

        if is_last_read:
            # The last chunk will be smaller than the others so the
            # line count needs to be calculated. b2a_base64 adds a line
            # break so we don't count that char
            base64_lines_per_read = int(ceil((len(data) - 1) / float(base64_line_size)))

        # Split the data chunks into base64_lines_per_read number of
        # lines, each 76 chars long.
        for l in xrange(base64_lines_per_read):
            is_last_line = l == base64_lines_per_read - 1
            pos = l * base64_line_size
            line = data[pos:pos + base64_line_size]
            output.write(line)

            if not (is_last_line and is_last_read):
                # The very last line will already have a \n because of
                # b2a_base64. The other lines will not so we add it
                output.write('\n')

I ended up using bytearray as an input and output buffer. If found that if the output was something that doesn't buffer output (like a socket), then writing 77 bytes at a time would be very slow. Also my original code rounded the read size to be advantageous for base64, but not advantageous for MongoDB. It's better for the read size to match the MongoDB chunk size exactly. So the input is read into a bytearray with the exact size passed in, but then read in smaller base64 size chunks.

def chunked_encode(
        input, output, read_size=DEFAULT_READ_SIZE, write_size=(base64.MAXLINESIZE + 1) * 64):
    """
    Read a file in configurable sized chunks and write to it base64
    encoded to an output file.

    Args:
        input (file): File like object (implements ``read()``).
        output (file): File like object (implements ``write()``).
        read_size (int): How many bytes to read from ``input`` at
            a time. More efficient if in increments of 57.
        write_size (int): How many bytes to write at a time. More efficient
            if in increments of 77.
    """
    # 57 bytes of input will be 76 bytes of base64
    chunk_size = base64.MAXBINSIZE
    base64_line_size = base64.MAXLINESIZE
    # Read size needs to be in increments of chunk size for base64
    # output to be RFC 3548 compliant.
    buffer_read_size = max(chunk_size, read_size - (read_size % chunk_size))

    input.seek(0)

    read_buffer = bytearray()
    write_buffer = bytearray()

    while True:
        # Read from file and store in buffer until we have enough data
        # to meet buffer_read_size
        while input and len(read_buffer) < buffer_read_size:
            s = input.read(read_size)
            if s:
                read_buffer.extend(s)
            else:
                # Nothing left to read
                input = None

        if not len(read_buffer):
            # Nothing in buffer to read, finished
            break

        # Base 64 encode up to buffer_read_size and remove the trailing
        # line break.
        data = memoryview(b2a_base64(read_buffer[:buffer_read_size]))[:-1]
        # Put any unread data back into the buffer
        read_buffer = read_buffer[buffer_read_size:]

        # Read the data in chunks of base64_line_size and append a
        # linebreak
        for pos in xrange(0, len(data), base64_line_size):
            write_buffer.extend(data[pos:pos + base64_line_size])
            write_buffer.extend('\n')

            if len(write_buffer) >= write_size:
                # Flush write buffer
                output.write(write_buffer)
                del write_buffer[:]

    if len(write_buffer):
        output.write(write_buffer)
        del write_buffer[:]

For 10 iterations of a 10 MB file (complete test), this version is up to 5 times faster than standard base64 with large buffer sizes (>969) when reading a file without buffering (like a socket). For small buffer sizes (~100) it is about the same or worse than standard base64.

--- bufsize 4096
standard_base64_encode 5.70770692825 seconds for 10 iterations
original_chunked_encode 2.07641100883 seconds for 10 iterations
latest_chunked_encode  1.44510507584 seconds for 10 iterations
--- bufsize 2048
standard_base64_encode 5.71355605125 seconds for 10 iterations
original_chunked_encode 2.17808198929 seconds for 10 iterations
latest_chunked_encode  1.5746011734 seconds for 10 iterations
--- bufsize 1024
standard_base64_encode 5.7339630127 seconds for 10 iterations
original_chunked_encode 2.35343503952 seconds for 10 iterations
latest_chunked_encode  1.83091807365 seconds for 10 iterations
--- bufsize  969
standard_base64_encode 5.87562203407 seconds for 10 iterations
original_chunked_encode 2.3832950592 seconds for 10 iterations
latest_chunked_encode  1.81391692162 seconds for 10 iterations
--- bufsize  100
standard_base64_encode 5.84305310249 seconds for 10 iterations
original_chunked_encode 6.96859192848 seconds for 10 iterations
latest_chunked_encode  6.85651683807 seconds for 10 iterations
--- bufsize   57
standard_base64_encode 5.72181987762 seconds for 10 iterations
original_chunked_encode 6.98394799232 seconds for 10 iterations
latest_chunked_encode  8.28728795052 seconds for 10 iterations


Create Binary, Balanced Tree I wrote a function, which takes a list of Integers, and then makes a balanced binary tree.

Please critique it. I'd like to know know how to make it more concise and idiomatic.

-- Note that the Integer represents the height of the Tree
data Tree a = Leaf
              | Node Integer (Tree a) a (Tree a)
              deriving (Show)-- where Integer is the height (bottom = 0)

-- make balanced, binary tree (height diff is <= 1)
-- Note - I would've liked to have kept `foldTree` point-free,
--        but I'm not sure how to do that since I need `xs` for `treeHeight`
foldTree :: [a] -> Tree a
foldTree xs = (foldingFn . zip [0..]) xs
   where foldingFn = foldr (\(i, elem) acc -> if (odd i) then insertFreeOrLeft  treeHeight elem acc
                                              else            insertFreeOrRight treeHeight elem acc) Leaf
         treeHeight = getBinTreeHt xs

-- get Binary Tree Height (used to start making the Tree)
getBinTreeHt :: [a] -> Integer
getBinTreeHt = floor . (logBase 2) . fromIntegral . length

-- insert where there's a Leaf, otherwise choose Left
insertFreeOrLeft :: Integer -> a -> Tree a -> Tree a
insertFreeOrLeft index x Leaf                    = Node index  Leaf x Leaf
insertFreeOrLeft _ x (Node level Leaf val right) = Node level (Node (level-1) Leaf x Leaf) val right
insertFreeOrLeft _ x (Node level left val Leaf)  = Node level left val (Node (level-1) Leaf x Leaf)
insertFreeOrLeft _ x (Node level left val right) = Node level (insertFreeOrLeft (level-1) x left) val right

-- insert where there's a Leaf, otherwise choose Right
insertFreeOrRight :: Integer -> a -> Tree a -> Tree a
insertFreeOrRight index x Leaf                    = Node index  Leaf x Leaf
insertFreeOrRight _ x (Node level left val Leaf)  = Node level left val (Node (level-1) Leaf x Leaf)
insertFreeOrRight _ x (Node level Leaf val right) = Node level (Node (level-1) Leaf x Leaf) val right
insertFreeOrRight _ x (Node level left val right) = Node level left val (insertFreeOrRight (level-1) x right)

Testing

*Main> foldTree "ABC"
Node 1 (Node 0 Leaf 'B' Leaf) 'C' (Node 0 Leaf 'A' Leaf)

*Main> foldTree "ABCDE"
Node 2 (Node 1 (Node 0 Leaf 'B' Leaf) 'D' Leaf) 'E' (Node 1 Leaf 'C' (Node 0 Leaf 'A' Leaf))

From the design perspective, I see these possible conceptual problems:

The data structure doesn't help in determining if the required invariant holds or not.
Tree holds information that is outside of its scope - its level in the tree. This means, among other things, that a node can't be shared between multiple trees, which is something you want when manipulating trees (a new tree is just a slightly modified version of an old one, sharing most of its nodes).
Your design focuses only on creating a tree from a list. If you don't need any other operations, that's fine, but if you want to do more operations later, like insertion/deletion, merging trees, etc., it's going to be problematic. In particular, for some operations, when the total height of the tree must change, you'll have to update the whole tree. Most likely, it'd be possible to find an arbitrarily long sequence of operations, each taking O(n) time (where n is the number of nodes in the tree).

All these problems relate to having the node's level in Tree, which is currently sort-of unused. If instead you kept there the height of the subtree rooted at a node, you would be able to:

Easily check that a function keeps the required invariant when creating/manipulating nodes.
Each Tree value would be self-contained, depending only on its own subtree, thus can be shared within multiple trees.
Following from 2., you can implement the operations on trees in such a way that each takes just O(log n).

Also I'd suggest you to have a look at various balanced tree implementations (unless you want to explore it yourself, which is a good exercise).

On the other hand, if your aim is only to have a balanced tree, without the need of modifying it later, you could somewhat relax your requirement, and just require that the height of the tree is at most 1+log{2} n. Then you'd be able to create the tree incrementally, without knowing the total length of the input list: at a particular point, the algorithm tries to fill a node of height h. If it's full and there are more incoming data, it starts to build another node of height h+1. So the left subtree will be always a full binary tree.

From the syntactical point of view, it's very good that you annotate all functions with their
types, the only thing I'd improve is to have a fixed line length limit to increase readability.


Monitoring the recursive growth of a zombie rabbit population Breeding like rabbits

  Write a function answer(str_S) which, given the base-10 string
  representation of an integer \$S\$, returns the largest n such that \$R(n) = S\$. Return the answer as a string in base-10 representation. If there
  is no such n, return "None". S will be a positive integer no greater
  than \$10^{25}\$

  where \$R(n)\$ is the number of zombits at time \$n\$:

  $$\begin{align*}
R(0) &= 1 \\
R(1) &= 1 \\
R(2) &= 2 \\
R(2n) &= R(n) + R(n+1) + n \quad\text{(for $n > 1$)} \\
R(2n+1) &= R(n-1) + R(n) + 1 \quad\text{(for $n \geq 1$}) \end{align*}$$

Test cases

>>> answer('7')
'4'
>>> answer('100')
None

My program is correct but it is not scalable since the range of \$S\$ can be a very large number like \$10^{25}\$.

Could anyone help me with some suggestions on improving the code further so that it can cover any input case?

def answer(str_S):

    d = {0: 1, 1: 1, 2: 2}
    str_S = int(str_S)
    i = 1
    while True:

        if i > 1:
            d[i*2] = d[i] + d[i+1] + i
            if d[i*2] == str_S:
                return i*2
            elif d[i*2] > str_S:
                return None

        if i>=1:
            d[i*2+1] = d[i-1] + d[i] + 1
            if d[i*2+1] == str_S:
                return i*2 + 1
            elif d[i*2+1] > str_S:
                return None

        i += 1

print answer('7')

Comments on your code:

I’m surprised by the requirement that the input to answer() has to be a string, not an int.

Since an int seems to be the natural input, I’d put most of the processing code in a separate function that only deals in ints, and do the string mangling at the very far edges of the program. Something like:

def answer(str_S):
    result = answer_from_int(int(str_S))
    if result is None:
        return None
    else:
        return str(result)

That allows you to separate the processing logic from the (somewhat unusual) interface.
There are no comments or docstrings in your code, which means that

I don’t know what it’s supposed to do, and
I don’t know why you wrote it this way.

Knowing either of those would make the code substantially easier to read, review and maintain. Get into the habit of writing comments and docstrings – when you’re debugging your old code, you’ll thank yourself for it.
Choose more descriptive variable names. You can usually find more appropriate names than single characters, and the result is significantly more readable code. It’s much easier to follow the control flow if I’m following names instead of letters.

Comments on your approach:

You’re right, this won’t scale. You’re caching a huge number of results, most of which aren’t used. I did some quick experiments, and found that \$R(n) \approx 2n\$, which means that a 10^25 lookup is going to require a dict with 10^25 entries. Ouch.

You’re building up to find \$S\$, even if \$S\$ is huge. We can approach it much faster.

Here are a few suggestions:

Computing a single value \$R(n)\$ only needs a small subset of the values \$R(1)\$ to \$R(n-1)\$. By using the recurrence relations, you can get to it in very few operations (I think O(log n).)

So rather than computing and caching every value you might need, only do that for the results you actually need. This will result in significant occupancy and processing savings.

But how do we know which values of \$R(n)\$ we should look at?
Let's split the sequence into odd and even results, which I’ll define with the notation
$$R_O(n) = R(2n) \qquad R_E(n) = R(2n+1)$$
Since all \$R(n)\$ are positive, both of these sequences are monotonically increasing. Further note that \$R(n) \geq n\$ for all \$n\$.

As such, I know that if there is some \$n\$ such that \$R(n)=S\$, it must have \$0 \leq n \leq S\$. So we can do a binary search in the odd/even subsequences for \$S\$, and quickly find out whether it’s in either of the sequences (and then which has the larger value of \$n\$).

Here’s a skeleton class I wrote that allows me to efficiently compute zombit counts. The code isn’t complicated, so you can try filling it out:

class Zombit(dict):

    def __init__(self):
        # set up the initial values

    def lookup(self, N):
        # Get the value R(N), either from the cache or by computing from
        # the recurrence relations.  Remember to cache the result!

I can use this to look up \$R(6\cdot10^{300})\$ in a fraction of a second and with minimal occupancy. You’ll easily be within the bounds of the question, and then some. (Trying to go higher, I ran into Python’s max recursion depth.)

Setting up the binary search is a bit harder, but still not too complicated.


Indices of true values My task is to find the indices of a vector of booleans that are true. I'm coming from a JavaScript background and would like to learn how to write this in idiomatic Rust.

fn main() {
    let fold_func = |acc: Vec<usize>, p: (usize, &bool)| -> Vec<usize> {
        if *p.1 {
            let mut clone = acc.clone();
            clone.push(p.0);
            clone
        } else {
            acc
        }
    };
    let v = vec![true,false,true];
    let result = v.iter().enumerate().fold(vec![], fold_func);
    print!("{:?}", result) //[0, 2]
}

Spaces come after commas

-    let v = vec![true,false,true];
+    let v = vec![true, false, true];

Define the closure inline in the fold call. This allows removing the explicit types
Cloning the accumulator is more expensive than it needs to be. You have been given ownership of the vector, just declare it mutable and push to it.

The code now looks like:

let result = v.iter().enumerate().fold(vec![], |mut acc, p| {
    if *p.1 {
        acc.push(p.0);
        acc
    } else {
        acc
    }
});

Extract out the common part of the closure that returns the acc
Destructure p to give the components names.

The code now looks like:

let result = v.iter().enumerate().fold(vec![], |mut acc, (index, value)| {
    if *value {
        acc.push(index);
    }
    acc
});

I advocate to basically memorize all the methods on Iterator. In this case, filter, map, and collect allow writing the entire thing more succinctly, equally efficiently, as well as more flexibly.
There's no need to allocate the Vec, an array / slice works the same.

The code now looks like:

fn main() {
    let v = [true, false, true];

    let result: Vec<_> = v.iter()
        .enumerate()
        .filter(|&(_, &value)| value)
        .map(|(index, _)| index)
        .collect();

    println!("{:?}", result);
}

  I would expect that filter() filters out non true values, so indices would shift, but it seems like it's not happening

Yes, filter does remove the non-true values. The important thing to recognize is where the indices are added.

With iter().enumerate().filter(), enumeration happens before the filtering. Each index is added based on an unfiltered iterator.

If the code had instead been iter().filter().enumerate(), the enumeration happening after the filtering, then the index would have been added based on the filtered iterator. This would result in the indices 0, 1, 2, ..., which isn't very useful, as you point out.


Read from DataInputStream (Socket connection) I am interfacing with a network device and must read a response after I issue a command. Sometimes when I attempt to read, nothing has been returned from the terminal. This means I must wait, but not forever, right?

My solution: Enter a loop with a time comparison guard (time elapsed since start). Then inside, I will check to see if there is anything available to read. If so read it and break of the loop if nothing left.

I couldn't figure out another approach. Having a while loop on the number of bytes read was fine up to the point where there was no data left, in which case it would just wait until the timeout to occur. My way will return as soon as everything has bead read or else timeout occurs.

Does this make sense? I currently have the timeout set to 10 seconds.

private Socket socket;
private DataInputStream reader;
private DataOutputStream writer;

socket = new Socket(device.getHostAddress(), device.getHostPort());

socket.setSoTimeout((int) READ_TIMEOUT_MILLIS);

reader = new DataInputStream(socket.getInputStream());
writer = new DataOutputStream(socket.getOutputStream());

writer.write(data.getBytes(ENCODING_CHARSET));
writer.flush();

ByteArrayOutputStream bufferStream = new ByteArrayOutputStream();

byte[] buffer = new byte[1024];

long opStartMillis = System.currentTimeMillis();

while(System.currentTimeMillis() - opStartMillis < READ_TIMEOUT_MILLIS) {

    if(reader.available() > 0) {

        int bytesRead = reader.read(buffer);

        bufferStream.write(buffer, 0, bytesRead);

        if(reader.available() == 0) {

            break;
        }
    }
}

callback.dataReceived(bufferStream.toString(ENCODING_CHARSET));

Too late for OP but anyway...

Seems that you read from socket continuously when data is available. If there is not incoming data in socket for some reason you can wait maximum 10 seconds (it's just an exemplary number) and then you terminate the program.

Some kind of loop is required anyway (because you read as much as possible). The loop can be hidden with aid of Java's Timer, but it will not make the code more clean.

Please consider the following example (I played with files instead of sockets, but from data stream's point of view they expose the same interface):

public class DataInputStreamWithTimeoutExample {
    public static final int TIMEOUT = 10;

    public static void main(String args[]) throws IOException, ExecutionException, InterruptedException {
        final DataInputStream reader =
                new DataInputStream(new FileInputStream("/home/me/Temp/java_example/in.txt"));
        final DataOutputStream writer =
                new DataOutputStream(new FileOutputStream("/home/me/Temp/java_example/out.txt"));

        final ExecutorService singleThreadExecutor = Executors.newSingleThreadExecutor();
        final Future<Void> future = singleThreadExecutor.submit(new Courier(reader, writer));

        try {
            System.out.println("Started...");
            future.get(TIMEOUT, TimeUnit.SECONDS);
            System.out.println("Finished.");
        } catch (TimeoutException e) {
            System.out.println("Timeout has gone!");

            // TIMEOUT seconds are passed, but infinite loop is not finished.
            // Lets send `interrupt` message to it.
            future.cancel(true);
            singleThreadExecutor.shutdown();
        }
    }
}

class Courier implements Callable<Void> {
    public static final int BUFFER_SIZE = 1024;

    private final DataInputStream reader;
    private final DataOutputStream writer;

    public Courier(final DataInputStream reader, final DataOutputStream writer) {
        this.reader = reader;
        this.writer = writer;
    }

    @Override
    public Void call() throws Exception {
        while (true) {
            int availableBytes = reader.available();
            if (availableBytes > 0) {
                final byte[] buffer = new byte[BUFFER_SIZE];
                int bytesRead = reader.read(buffer);
                writer.write(buffer, 0, bytesRead);
            }
            // We need analyze whether someone decided
            // to interrupt the infinite loop.
            if (Thread.interrupted()) {
                writer.flush();
                writer.close();
                System.out.println("Terminated.");
                return null;
            }
        }
    }
}

We analyze status of the thread inside infinite loop because without such check it will be impossible to interrupt the loop outside it.

You can ask Where is the gain?

We wiped out System.currentTimeMillis()... stuff. Java's ExecutorService does calculations of time for us.
Timeout is set outside the loop (please note that there are not any references to TIMEOUT constant inside Courier class). So Courier class can be re-used in another program (but don't forget that you need to send interrupt message to Courier with aid of future.cancel(true); and singleThreadExecutor.shutdown();, otherwise the program will never end).

PS while working on this answer I used awesome answers:

"How to timeout a thread"
"How to Stop long duration execution task ( e.g. Infinite Loop) execution inside a Executor Service"


Calculate the continued fraction using the function indicators I'm developing programs on Golang. But now you have to study at the university and have the following task.

  Calculate the continued fraction using the function indicators. The
  number of fractional elements must be specified from the keyboard. 1 +
  1 / (1 + 1 / (1 + 1 / (1 + 1 / ....))). The number of elements of the
  fraction must be at least 5. Consider three cases, for example, 5, 7
  and 10.

Enter the number of elements of the continued fraction: 6
Solution - 1.61538

Please check my code.

#include <iostream>
using namespace std;
int main(){

    int n;
    double t;
    t=2;
    bool exit;

    while (exit != true) {
        printf("To exit the programs, type 0\n");
        cout << "Enter the number of continued fractions: " << endl;
        cin >> n;

        if (n == 0) {
            exit = true;
        } else {
            if (n < 5) {
                printf("The number of elements of a fraction must be at least 5");
                cin.get();
            } else {
                for (int a = 1; a < n; a++) {
                    t = 1 / t + 1;
                }
                cout << "Decision - " << t << endl;
            }
        }
    }
    printf("Exiting the program");
    return 0;
}

Don't use using namespace std
Prefer prefix over postfix
return 0 is not needed at the end of main
Prefer using \n over std::endl
Don't use printf in C++ code
As Reinderien already mentioned the exit variable is not needed. I.e.

for (;;) {
        // [...]
        if (n == 0) {
            break;
        }
        // [...]

will be sufficient.


Program to calculate Hash and Size of a torrent I wrote this small program to open a .torrent file, retrieve its info-hash and size. I'm still a beginner at Python so the main focus of this program was to try and utilize classes and objects instead of having just a bunch of functions. It works but I wanted to know if this is a good design. Also, I feel some of the code I wrote is redundant especially the number of times self is used.

import hashlib, bencode

class Torrent(object):

    def __init__(self, torrentfile):
        self.metainfo = bencode.bdecode(torrentfile.read())
        self.info = self.metainfo['info']
        self.files = self.metainfo['info']['files']
        self.md5hash = self.md5hash(self.info)
        self.size = self.size(self.files)

    def md5hash(self, info):
        return hashlib.sha1(bencode.bencode(info)).hexdigest()

    def size(self, files):
        filesize = 0
        for file in files:
            filesize += file['length']
        return filesize

torrentfile = Torrent(open("test.torrent", "rb"))
print(torrentfile.md5hash)
print(torrentfile.size)

Using classes for this seems like a good idea, since you end up with a number of class instances, where each one represents a specific torrent.

In terms of the specific code, you're doing things slightly wrong in 2 ways.

Firstly, you don't need to pass instance parameters into the methods of a class. So you can access info and file as self.info and self.file, so your methods only need the self argument.

Secondly, I can see that you're doing this to try to cache the results of the method calls by overriding the methods in __init__, and while caching is good, this is a bad way of trying to achieve it.

There are 2 alternatives that spring to mind, depending on what you want to do:

If you always want the size and hash calculated when the class is instantiated, then do something similar to what you're doing now, but use different names for the data variables and the methods:

def __init__(self, torrentfile):
    self.metainfo = bencode.bdecode(torrentfile.read())
    self.info = self.metainfo['info']
    self.files = self.metainfo['info']['files']
    self.md5hash = self.calculate_md5hash()
    self.size = self.calculate_size()

def calculate_md5hash(self):
    return hashlib.sha1(bencode.bencode(self.info)).hexdigest()

def calculate_size(self):
    filesize = 0
    for file in self.files:
        filesize += file['length']
    return filesize

Alternatively, if you only want the hash and size calculated when the methods are specifically called, but you also want caching, use lru_cache

lru_cache will cache the result of a function the first time it is run, and then simply return the result for future calls, providing the arguments to the function remain the same.

from functools import lru_cache

class Torrent(object):

    def __init__(self, torrentfile):
        self.metainfo = bencode.bdecode(torrentfile.read())
        self.info = self.metainfo['info']
        self.files = self.metainfo['info']['files']

    @lru_cache()
    def md5hash(self):
        return hashlib.sha1(bencode.bencode(self.info)).hexdigest()

    @lru_cache()
    def size(self):
        filesize = 0
        for file in self.files:
            filesize += file['length']
        return filesize

Then call the methods explicitly:

print(torrentfile.md5hash())
print(torrentfile.size())


Readability concerns with literal dictionary lookup vs if-else I'm having concerns about the readability of this piece of code:
    messages_dict = {'error':errors, 'warning':warnings}[severity]
    messages_dict[field_key] = message

and I'm to use this instead:
    if severity == 'error':
        messages_dict = errors
    elif severity == 'warning':
        messages_dict = warnings
    else:
        raise ValueError('Incorrect severity value')
    messages_dict[field_key] = message

But it looks too verbose for such a simple thing.
I don't care too much if it's more efficient than constructing a dictionary for just two mappings. Readability and maintainability is my biggest concern here (errors and warnings are method arguments, so I cannot build the lookup dictionary beforehand and reuse it later).

Since you mention maintainability I think that the consideration can be taken in terms of what next changes may look like. If further changes will take more statements to be executed with the result of one of those specific options, or if more options will be added, I would stick to the dictionary approach.

Looking into the future can certainly be tricky, but you need to also think how much will the code need to be change to assert a specific new need. If you need to add a new level of reporting, would you prefer to add a new else if or just a value in a dictionary?

Readability then starts to backfire, since if ... else is quite common. What happens when you arrive at 3, 4 or 5 branches?


XML parsing program in C++ This is my code which works and compiles but I feel it may be non-optimal, especially the tag mapping and  use of the if command.

It reads two .xml files and finds what has been added or deleted on the new file. For new items it uses a map to link a tag name to a category of data (content tag is a description for example). This is important since different files have different naming conventions on the tags.

Finally, it does work on the data depending on its category (coming soon) and prints it.

#include "pugi/pugixml.hpp"

#include <iostream>
#include <string>
#include <map>

int main()
{

    std::string var;

    const std::map<std::string, std::string> tagMap {
        {"id", "id"}, {"description", "content"}, {"url", "web_address"}
    };

    pugi::xml_document doca, docb;
    std::map<std::string, pugi::xml_node> mapa, mapb;

    if (!doca.load_file("a.xml") || !docb.load_file("b.xml")) {
        std::cout << "Can't find input files";
        return 1;
    }

    for (auto& node: doca.child("data").children("item")) {
    const char* id = node.child_value("id");
    mapa[id] = node;
    }

    for (auto& node: docb.child("data").children("item")) {
    const char* idcs = node.child_value("id");
        if (!mapa.erase(idcs)) {
        mapb[idcs] = node;
        }
    }

     for (auto& eb: mapb) {
        // Loop the tagMap, and try and associate the tag name to the content
         for (auto& kv : tagMap) {
             if (kv.first == "id") {
             std::string var = eb.second.child_value(kv.second.c_str());
             // Do work on returned value of id tag in the future (I.e validation)
             std::cout << var << endl;
             }
             if (kv.first == "description") {
             std::string var = eb.second.child_value(kv.second.c_str());
             // Do work on returned value of content tag in the future
             std::cout << var << endl;
             }
             if (kv.first == "url") {
             std::string var = eb.second.child_value(kv.second.c_str());
             // Do work on returned value of web_address tag in the future
             std::cout << var << endl;
             }
         }
     }
}

I will only comment about the following not-fully-implemented part.

 for (auto& kv : tagMap) {
     if (kv.first == "id") {
     std::string var = eb.second.child_value(kv.second.c_str());
     // Do work on returned value of id tag in the future (I.e validation)
     std::cout << var << endl;
     }
     if (kv.first == "description") {
     std::string var = eb.second.child_value(kv.second.c_str());
     // Do work on returned value of content tag in the future
     std::cout << var << endl;
     }
     if (kv.first == "url") {
     std::string var = eb.second.child_value(kv.second.c_str());
     // Do work on returned value of web_address tag in the future
     std::cout << var << endl;
     }
 }

Even without much code filling the blanks, it seems that you want to have some associative tag => function data. In which case, you could create an std::unordered_map with strings as keys and associating function pointers to them, generated from captureless lambdas. A small example:

std::unordered_map<std::string, void(*)(const std::string&)> tasks = {
    { "id", [](const std::string& val) {
        // Do work on returned value of id tag
    }},
    { "description", [](const std::string& val) {
        // Do work on returned value of content tag
    }},
    { "url", [](const std::string& val) {
        // Do work on returned value of web_address tag
    }}
};

for (auto& kv: tagMap) {
    auto it = tasks.find(kv.first);
    if (it != tasks.end()) {
        it->second(eb.second.child_value(kv.second.c_str()));
    }
}

That way, your code is easy to extend to other tags by representing tasks associated to tags as data and storing them into a map. Now, it will only work if you don't need captures, otherwise, you may have to use std::function or some more heavyweight mechanism instead of a big chain of if.


Get lowest integer that is not in set Is there a better (functional or more idiomatic) way to get the first positive number that is not included in a set?

val set = Set(4, 1, 2, 0, 5)
var x = 0
while(set(x)) x += 1
println(x) // Should print 3

You could try for a more functional expression that matches problem description more closely:

 Stream.from(1).filter(!set.contains(_)).head

[Caveat: I'm not a scala programmer, I have no idea how idiomatic this is]