Enigma questions from "ANSI Common Lisp" I'm working through Paul Graham's ANSI Common Lisp and in Exercise 2.5, I'm stumped. My specific question (I wasn't sure whether this type of question is suitable for stack overflow, code review, or neither) is:

How to even begin working on an answer to this question.

What's interesting is that I simply don't know how to proceed, analytically. Hopefully, it isn't a question which involves simply knowing the answer due to existing familiarity. So, my question is subtle, and it's not simply to ask 'what is the answer', but 'how could you go about finding the answer', if you were really stumped. So I'm asking about method.

The Question: What do these functions do?

(defun enigma (x)
  (and (not (null x))
       (or  (null (car x))
            (enigma (cdr x)))))

(defun mystery (x y)
  (if (null y)
      nil
      (if (eql (car y) x)
          0
          (let ((z (mystery x (cdr y))))
            (and z (+ z 1))))))

I've realised in typing this in that one approach would be simply to run the functions. Perhaps we could call that the "what does this button do?" approach. :)

Update: before posting this, I've found I just solved the first one.
The method I used (I won't reveal the answer completely here, but the following will certainly make it much easier to guess): just run the function with various inputs. First I discovered it errors unless the argument is not a list (we see it calls car on the arg). So, the first arg to and looks like a base case. While it's clear to me now what it does (easy to say), the only way I was able to discover it was by playing with various inputs. It seemed always to return nil on any input I gave it. That seemed strange. Then I just looked at the line (or  (null (car x)), and it's easy to see how to make that evaluate to true: put a nil as an element of the list. And then the secret was revealed... This seems an interesting function, worth plenty of attention. I do love recursion :)

The second remains mystery for now. Please remember, I'd appreciate discussion to be around method and/or techniques, if anyone would care to chip in. Obviously there's much less value in just revealing the answer.

I'm also huge fan of recursion, and the best book on recursion I know is "Little Schemer". I highly recommend it! It looks "slow" and repetitive, but that's how you learn (at least this approach was great for me).

In my opinion it's hard to read code when you find pattern you didn't knew before. It takes a lot of time and is highly frustrating to figure out what's happening inside, and often you still don't fully get why it was written this way (and if it's even correct). So the best advise I can give is - learn how to write code, reading will come naturally. You know what you are looking for.

In recursion, most important part is - when to stop and what to do (return) in that case.

Your mystery function stops when:

y is nil (it returns nil) - first if
or
first element of y is eql to x (it returns 0) - second if

It will also return logical "something and something" - the last line - but we will talk about it later.

From the above, we know that:

y should be some list, since it's user with car and cdr functions, and tested for empty list (null y)
x should be some atom, simple one, since it's compared to every element of y list with eql, which works fine with things like symbols and numbers, but not more complicated things like strings or lists

OK, we know what to pass to mystery functions (roughly). Now - what this last part with let and and do? Apparently let is used here to store result of recursive call only to speed thing up. It could also be written as:

(and (mystery x (cdr y))
     (1+ (mystery x (cdr y))))

We know what mystery function can return: nil or 0. Last thing to do is to analyse what and can return:

first argument if it evaluates to false
second argument otherwise.

As you can see there is no magical trick to help reading code. When you use some pattern (recursion in this case) long enough, you learn what are the important things to look for, and what are just common construction parts.

When you have some idea how function works, you can call it with some parameters just to verify your understanding, but I don't think you should begin with that. Pan and paper method can be a lot better, especially when you start with simple cases, like what will happen if I pass an empty list and empty list? Or zero and one element list? Calling function you don't know and observing results is called "black box testing" and is more hackerish activity. You have all the information you need - code of the function - so it's easier to read and reason about its behaviour.

When you stuck and don't know how to proceed, try to reformat the code. Rewrite the parts you understand. It might look silly and like wasting your time, but I find it helpfull.

If something is not clear, I'm happy to expand my answer, just post a comment :)


Groovy: permutations() with size limit I have a problem to solve. My algorithm is assigning Players to Positions. On the start I have a list of about 16 positions and 4 players. The problem is to assing every player to it's closest positions so the overall distance is as low as possible.

I've already done this:

List players = ...
List positions = ...
Set posPerms = positions.permutations().collect { it.subList(0, playmakers.size()) } as Set
List assignation = posPerms.min { List perm ->
    int v = 0
    perm.eachWithIndex { Position pos, int index ->
        v += pos.distance(players[index])
    }
    v
}
[players, assignation].transpose().collectEntries { it }

I get what I want, but the performance is a problem. positions.permutations() generates massive lists. Is there any more optimal way to do this?

If you have always at most 4 player and the number of positions is smaller than 100 (like your 16 start positions), then checking all possible position and players takes 1004, which is small enough. but if you looking for faster solution (harder implementation) You can use Maximum Weighted Matching by Edmound, First you should create a graph, you can draw graph, by setting

Vertices = players position Union available position as nodes.
Edges = draw edge between each player and all available positions

For setting weight to edges you should do it in reverse, In fact first find the maximum distance (between all possible (player , position) pair, name it MaxDistance, now set your graph edges as MaxDistance - EdgeDistance + 1, here EdgeDistance is distance between player and desired position.

You can find good implementation of Edmond's algorithm in c++ in Lemon package.


Find the next 8 dates that are Mondays (exclusive of today) Any thoughts on improving this method of finding the next 8 Mondays not including today?

var app = angular.module("angularApp", []);
app.controller("appController", function($scope, $filter) {
  $scope.mondays = [];

  var MILLISECONDS_IN_A_DAY = 1000 * 60 * 60 * 24,
    now = new Date(),
    nowTime = now.getTime(),
    mondayOffset = 7 - (now.getDay() - 1);

  for (var i = 0; i < 8; i++) {
    $scope.mondays.push(
      new Date(nowTime + ((MILLISECONDS_IN_A_DAY * mondayOffset) + (MILLISECONDS_IN_A_DAY * 7 * i)))
    );
  };
});
body {
  font-family: verdana;
  font-size: 10pt;
}

{{date | date}}

I think you may have a bug (depends on how the calculation should work). If today is a Sunday, the mondayOffset will be 7 - (0 - 1) == 8, meaning the entire upcoming week will be skipped. In other words, mondayOffset will always be in the range 2-8. I'm guessing that's not what you want.

When dealing with dates, I find it simpler to deal with them as just dates, rather than as millisecond timestamps. Also, with timestamp arithmetic there's always the risk of time zones messing things up for you (for instance when going to/from daylight savings time).

Besides a millisecond number, the Date constructor also accepts separate arguments for year, month, date, hour, etc.. The neat thing is that if you pass a date that doesn't exist, like June 33rd, it'll automatically roll over to July 3rd.

So you can just increment the date argument as needed to produce valid dates in the future:

var app = angular.module("angularApp", []);
app.controller("appController", function($scope, $filter) {
  $scope.mondays = [];

  var today  = new Date(),
      year   = today.getFullYear(),
      month  = today.getMonth(),
      date   = today.getDate(),
      offset = 8 - (today.getDay() || 7); // days till next Monday

  for(var i = 0 ; i < 8 ; i++) {
    $scope.mondays.push( new Date(year, month, date + offset + 7 * i) );
  }
});
body {
  font-family: verdana;
  font-size: 10pt;
}

{{date | date}}

Here the offset will be in the range 1-7, so if today's a Sunday, it'll only skip 1 day ahead.


A way to do this without a lot of variables and casting? Currently I have this code:

float tileX = (float)rectangle.X / (float)newTileSize;
float tileY = (float)rectangle.Y / (float)newTileSize;

int xOrigin = (int)Math.Round(tileX) * newTileSize;
int yOrigin = (int)Math.Round(tileY) * newTileSize;

// Same idea for the next 4 lines

float tileWidth = (float)rectangle.Width / (float)newTileSize;
float tileHeight = (float)rectangle.Height / (float)newTileSize;

int selectorWidth = (int)Math.Round(tileWidth) * newTileSize;
int selectorHeight = (int)Math.Round(tileHeight) * newTileSize;

The reason for the first two lines is because if I don't cast them as a float it does integer division and I was a floating point value instead so I can round it in the next two lines.

This code is fine I guess it just seems like there would be a better way to do it without so much casting. Just seems rather verbose if that makes any sense.

Thanks.

So you're looking for

int result = (int)Math.round((float)n/(float)d) * d;

(assuming you are only interested in the intermediate values for this particular calculation)

Here's a test case with an alternative:

import junit.framework.TestCase;

public class TestModulo extends TestCase {

    private final static int d = 7;

    public void testModulo() throws Exception {
        for (int n = 0; n < 30; n++) {
            assertEquals("fails for " + n, f2(n), f1(n));
        }
    }

    private int f1(int n) {
        return (int) Math.round((float) n / (float) d) * d;
    }

    private int f2(int n) {
        return ((n + d / 2) / d) * d;
    }

}

You can solve your problem using only integer math.


`ls` indicates when directory is empty/full of dotfiles I want ls to print a message when run on an empty or full of dotfiles directory. Instead of:

$ ls empty_dir/ dotfiles/
$

I want:

$ ls empty_dir/ dotfiles/
empty_dir is empty

dotfiles contains only hidden files
$

If dir contains only dotfiles, and if the ls command is able to show them, the following should appear:

$ ls dotfiles/
dotfiles contains only hidden files
$ ls dotfiles/ -A
.hidden_dir

I searched a solution for that behavior, but can't found anything, so end up to implement it myself, using zsh:

# Wrapper around ls function, printing dedicated messages
#  if directory is empty/full of hidden files.
#  Parse parameters in order to call verbosels_onefile correctly.
function verbosels() {
    # extract options and filepaths
    ls_options=""
    ls_filepaths=""
    for parameter in "$@"
    do
        #echo "PARAM: $parameter"
        # it's an option if it's start with a dash
        if [[ "$parameter" =~ ^-.* ]]; then
            #echo "OPTION"
            ls_options="$ls_options $parameter"
        else  # it's a path: call ls on it
            #echo "FILEPATH"
            # get escaped version of given filename
            filepath=$(print -r -- "${(q)parameter}")
            #echo "filepath: $filepath"
            ls_filepaths="$ls_filepaths $filepath"
        fi
    done
    # remove options surrounding spaces
    ls_options=$(echo "$ls_options" | tr -d "[:space:]")
    isnotfirst=  # set to true after the first iteration
    # call verbosels_onefile for each filepath
    for filepath in ${(z)ls_filepaths}
    do
        # print space only between ls calls
        if [ "$isnotfirst" ]; then
            echo ""  # line jump
        else
            isnotfirst=1
        fi
        #echo "CMD: |verbosels_onefile $filepath $ls_options|"
        verbosels_onefile $filepath $ls_options
    done
}

# Perform an ls call on only one directory/file, that must be in first parameter.
# The other parameters remain untouched.
# The ls call take into account any aliases on ls.
# This function is called by the higher level verbosels function.
function verbosels_onefile() {
    if [ "$1" ]; then
        1="$1"
    else
        1="."
    fi
    if [ -d "$1" ]; then
        # contains files (hidden included)
        if [ -n "$(command ls -A "$1")" ]; then
            if [ -n "$(command ls "$1")" ]; then
                ls "$@"
            else
                # the directory is not empty, and contains only hidden files:
                # print message only if the ls command returns nothing
                # NOTE: run the ls command twice. Could be costly.
                if [ -n "$(ls "$@")" ]; then
                    ls "$@"
                else
                    echo "$1 contains only hidden files" 1>&2
                fi
            fi
        else
            echo "$1 is empty" 1>&2
        fi
    elif [ -e "$1" ]; then
        ls "$@"
    else
        echo "$1 doesn't exists" 1>&2
    fi
}

Remarks:

seems over-complicated
non-perfect handling of some ls options, notably -l in case of only-dotfiles directory (print total 0)
if a directory contains only dot files, the ls command is runned twice. Could be costly if the directory contains a lot of dot files.

The following is not valid, because ls formatting (colors, columns,…) are not kept:

ls_result=$(ls "$@")
if [ "$ls_result" ]; then
    echo "$ls_result"
else
    echo "$1 contains only hidden files" 1>&2
fi

I'm using zsh, because it provides some easier treatment. A solution involving only generalist bash could be better because of the portability.
I'm looking for any readability/efficiency improvement, eventually for modules/programs that already do the job.

Portability issues

The function collects the given options and when it finally calls ls,
it puts the options at the end.
Unfortunately this doesn't work on OSX,
where the options must come before the filenames.

Your remarks

  seems over-complicated

Yes. Unfortunately, to get the behavior that you want,
I don't think this can get much simpler.

  non-perfect handling of some ls options, notably -l in case of only-dotfiles directory (print total 0)

Unfortunately, the only way to avoid that will only make the script even more complicated.

  if a directory contains only dot files, the ls command is runned twice. Could be costly if the directory contains a lot of dot files.

Perhaps you forgot to count the 2 runs of command ls in the if-else chain.
So in fact for each file the ls command is executed 4 times.

Filenames with spaces

This will not work when the filenames have spaces:

ls_filepaths="$ls_filepaths $filepath"

You can make it work with filenames with spaces,
and at the same time cleaner,
by using arrays.

"Remove options surrounding spaces"

I'm not sure what's going on here:

# remove options surrounding spaces
ls_options=$(echo "$ls_options" | tr -d "[:space:]")

For example -l -a would become -l-a which will not work.

Avoid negatives in variable names

It's generally not recommended to use negatives in variable names like isnotfirst,
because it could lead to strange conditions like not isnotfirst,
which is hard to read and confusing.
I suggest to rename it to first and use ! "$first" in conditions for a negative meaning.

Terminology

# Wrapper around ls function, printing dedicated messages

ls is not a "function", it's a command.
For example, verbosels is a function.

Looping over "$@"

When looping over "$@",
you can omit the "$@".
So instead of this:

for parameter in "$@"; do

You can write simply:

for parameter; do

Pattern matching

Instead of matching by regular expressions like this:

if [[ "$parameter" =~ ^-.* ]]; then

It would be slightly simpler to use pattern matching like this:

if [[ "$parameter" == -* ]]; then

Setting a variable to empty

Instead of this:

ls_options=""
ls_filepaths=""

You can simplify as:

ls_options=
ls_filepaths=

echo is the same as echo ""

You can replace echo "" with simply echo with no parameters.


Pushing all non empty items to the end of a list I have a list like this

list_ = [" ", " ", "X", " ", " ", "Z", " ", "Y", " "]

I am writing a function that will result in

list_ = [" ", " ", " ", " ", " ", " ", "X", "Z", "Y"]

Basically it will push all the non-empty (I'll call " " as empty here for simplicity) to the end of the list while retaining their orders

My solution as follow

def push(list_):
    def find_current_element(list_, from_):
        for i in range(from_+1, len(list_)):
            if list_[i] != " ":
                return i
        return None

    list_ = list_[::-1]
    good_index = list_.index(" ")
    current_element_index = find_current_element(list_, 0)
    while(current_element_index is not None):
        for i in range(good_index, len(list_)):
            if list_[i] != " ":
                list_[good_index], list_[i] = list_[i], " "
                good_index += 1
                current_element_index = find_current_element(list_, current_element_index)
    return reversed(list_)

It works, but it seems too long and using too much resource (reverse the list twice and bunch of for loops). I wouldn't care if the items were just simple character, but I plan to have some big objects as items of the list. I am wondering is there a simpler approach

Your code is incorrect, for example for input [' ', 'X', 'Y'] you get into an infinite loop.

You could just sort by emptiness, which with your apparent definition can be done like this:

>>> list_ = [" ", " ", "X", " ", " ", "Z", " ", "Y", " "]
>>> list_.sort(key=' '.__ne__)
>>> list_
[' ', ' ', ' ', ' ', ' ', ' ', 'X', 'Z', 'Y']

Or more generally (though slower) with a lambda or other self-made function:

list_.sort(key=lambda x: x != ' ')
list_.sort(key=is_non_empty)
list_.sort(key=is_empty, reverse=True)

If you actually had empty strings instead of strings containing a space character, or in general if your objects followed Python's usual thing of empty objects being false and non-empty objects being true, then you could also do this:

list_.sort(key=bool)


Autoloader class for PHP I created a class that I plan on using to autoload my project classes (20-30 classes) maximum. I wonder if there is anything, anything at all, that I can improve on this class to improve it any further.

Please let me know your thoughts below.

<?php
/**
 * YodaCMS, content management system.
 *
 * @info         This class autoloads project classes.
 * @author       Someone Someone <example@something.com>
 * @version      0.0.1
 * @package      YodaCMS
 *
 */

class autoLoader {
    private static $loadedLibrarys;

    public function __construct() {
        $loadedLibrarys = array();
    }

    public static function loadLibrarys() {
        newLibrary("testClass", new testClass());
    }

    private function newLibrary($lName, $lValue) {
        if (!$this->tryLoadLibrary($lName, $lValue)) {
            print_r('Error loading library: ' . $lName);
        }
    }

    public function tryLoadLibrary($lName, $lValue) {
        include(ROOT . '/applocation/base/classes/'.$lName.'.class.php');
        $this->loadedLibrarys[$lName] = $lValue;
        return true;
    }
}

I don't think that you need newLibrary and tryLoadLibrary. The names don't really make clear what the distinction is, and I can't imagine that you'd ever want to load a library but not know if it failed or not.

Also, instead of printing, I would throw an exception. That way, the calling code can handle the case (what if it's an irrelevant library? Now the user is still annoyed with a meaningless message; but what if it's a highly relevant library? Is a small message really enough?).

I would also get rid of the distinction between lName and lValue. If you follow conventions, they should be the same anyways. If they are not, it might get confusing, especially because it's not just this call, but also how they are stored in loadedLibrarys[$lName] = $lValue;,

I would also rename tryLoadLibrary to loadAdditionalLibrary. try is not really needed (we can assume that it will only try), and additional makes it more fitting with loadLibrarys. lName and lValue are also not all that clear; if I didn't read your code, I'd have no idea what l is, or what name and what a value is. libraryClassName and libraryFileName would be clearer (you can also skip the library part, as it's clear from context.

Finally, I would always check for directory traversal when including with a variable. It shouldn't really be necessary in this situation, but it's just good practice, and doesn't cost that much.


Luhn Algorithm in C++ How can I improve or shorten my implementation of the Luhn algorithm?

    int digits;
    cout << "enter the numer of digits" << endl;
    cin >> digits;
    int Acc_num[digits];
    int shuma = 0;

The shuma variable is the sum.

I created an array to store the digits from the string credit_card.

And asked the user how many digits his credit card has, etc. (the digits int).

    string credit_card;
    cout << "enter the identification number" << endl;
    cin >> credit_card;

    // store the digits
    for (int i = 0 ; i < digits ; i ++ ) {
        Acc_num[i] = credit_card[i];
    }

    for (int i = 0 ; i <= (digits - 1) ; i ++ ) {
        Acc_num[i] -= 48;
    }

    // Double every other
    for (int i = 1 ; i <=digits ; i ++) {
        if (i % 2 == 0) {
            Acc_num[i-1] =  2 * Acc_num[i-1];
        } else {
            Acc_num[i-1] = Acc_num [i-1];
        }
    }

    //Sum digits
    for (int i = 1 ; i <= digits ; i ++ ) {
        if (Acc_num[i-1] > 9 && i % 2 == 0) {
            int mod = Acc_num[i-1] % 10 ;
            Acc_num[i-1]  =  1 + mod ;
        } else {
        Acc_num[i-1] = Acc_num[i-1];
        }
    }

    // the sum
    for (int i = 0 ; i <= (digits - 1) ; i ++ ) {
        shuma += Acc_num[i];
    }

    if (shuma % 10 == 0) {
        cout << "\nthis number is valid" << endl;
    } else {
        cout << "\nthis number is invalid" << endl;
    }

Doubling the even digits and subtracting 9 if > 10 would be mod 9 so a really simple, compact algorithm might be:

for (int i = 0; i < nDigits; i++)
{
    int   digit = digitArray [i] - '0';   // Char to number
    if  (i & 1)  // Digit 1, 3, 5 not 0, 2, 4 - "even digits" starting at 1
        if  ((digit <<= 1) >= 10) //  Double it, check >= 10
            digit -= 9;           //  Same as summing the digits
    shuma += digit;
}
int checksum = shuma % 10;


Verilog UART Transmitter This is one of the first Verilog programs I have written.  I have a Xilinx Artix-7 FPGA card. Right now I just have it transmitting an "X" every second.  It works and I can see the result in my serial terminal.  It uses a UART over USB connection.

I'm just wondering if I can get some feedback on my code and if you see any problems.

module uart_top(input clk,
                input rx,
                output tx);

    reg [31:0] count = 0;
    wire ready;
    uart_send sender("X", count == 100000000, clk, tx, ready);

    always @(posedge clk)
        if(count == 100000000) count <= 0;
        else count <= count + 1;

endmodule

module uart_send #(parameter BAUD_RATE = 9600,
                   parameter CLOCK_SPEED_MHZ = 100)
                  (input [7:0] data_byte,
                  input start_send,
                  input clk,
                  output tx,
                  output ready);

        parameter integer CYCLES_WAIT = CLOCK_SPEED_MHZ * 1e6 / BAUD_RATE;

        parameter IDLE = 0;
        parameter START_BIT = 1;
        parameter END_BIT = 2;
        parameter DATA_BIT = 3;

        reg [2:0] state = IDLE;
        reg [15:0] cycle_count = 0;
        reg [3:0] bit_index = 0;
        reg [7:0] data;

        assign tx = state == IDLE ? 1 :
                    state == START_BIT ? 0 :
                    state == END_BIT ? 1 :
                    data[bit_index];

        assign ready = state == IDLE;

        always @(posedge clk) begin
            if(state != IDLE)
                data <= data_byte;
                if(cycle_count == CYCLES_WAIT) cycle_count <= 0;
                else cycle_count <= cycle_count + 1;

            if(state == IDLE && start_send) begin
                state <= START_BIT;
                cycle_count <= 0;
            end else if(state == START_BIT && cycle_count == CYCLES_WAIT) begin
                state <= DATA_BIT;
                bit_index <= 0;
            end else if(state == DATA_BIT && cycle_count == CYCLES_WAIT) begin
                if(bit_index == 7) state <= END_BIT;
                else bit_index <= bit_index + 1;
            end else if(state == END_BIT && cycle_count == CYCLES_WAIT) begin
                state <= IDLE;
            end
        end

endmodule

I would recommend your outputs be flops. It would be a cleaner output signal and easier to do timing analyses. It takes time (few nano seconds) to calculate the tx value after the clock with combinational logic. During that time the intermediate values are being transmitted and causing noise. This noise will can get worse from resistance an capacitance in your USB cable; lowers the signal quality. It seems your running at low speeds, so you wouldn't see a problem, but at higher speeds you may run into trouble.

Be aware that many synthesizers treat conditional operators (the ? : syntax) as explicit 2:1 muxes. So your current tx logic will always be a chain of 2:1 muxes even when a 4:1 is more suitable. A case statement within an always block tend to synthesize more optimally. I have a more detail answer for ? : vs case() on here on Electronics StackExchange.

Usually a FSM is written as a case statement; not if-else statements. The synthesizer doesn't really care. It is more of a common practice with RTL designers. It makes the FSM easier to identify and control how it synthesizes (full_case, parallel_case, onehot, encoding, etc.).

For a more optimzed design, your code should look something like the below. Or you can use two always blocks (my preference); one for calculating the next states for the flops, the other for simple flop assignment.

module uart_send #(parameter BAUD_RATE = 9600,
                   parameter CLOCK_SPEED_MHZ = 100)
                  (input [7:0] data_byte,
                  input start_send,
                  input clk,
                  output reg tx, // <-- reg outputs
                  output reg ready); // <-- reg outputs

  /* ... local parameter and reg declarations here ... */

  always @(posedge clk) begin
    case(state)
      IDLE : begin
          /*your IDLE code here*/
        end
      START_BIT : begin
          /*your START_BIT code here*/
        end
      END_BIT : begin
          /*your END_BIT code here*/
        end
      DATA_BIT : begin
          /*your DATA_BIT code here*/
        end
    endcase
  end

endmodule

I will also suggest changing the uart_send instantiation form connect by port order to connect by name.

uart_send sender("X", count == 100000000, clk, tx, ready);

to (port order does not matter)

uart_send sender( .data_byte("X"), .start_send(count == 100000000),
                  .clk(clk), .tx(tx), .ready(ready) );

or even cleaner (width matching)

wire [7:0] data_byte = "X";
wire start_send = (count == 100000000);
uart_send sender( .data_byte(data_byte), .start_send(start_send),
                  .clk(clk), .tx(tx), .ready(ready) );

I believe you can abbreviate matching name and size to just .name (ex .clk is the same as .clk(clk)) but I do not remember if that if Verilog or an added feature in SystemVerilog. I not have my copy if the LRMs with me.

wire [7:0] data_byte = "X";
wire start_send = (count == 100000000);
uart_send sender( .data_byte, .start_send, .clk, .tx, .ready );


Map error messages to conditions I have a method which checks some preconditions and if everything is fine returns a success response object, otherwise fail with an appropriate error message.

My problem is that I have a bunch of if-elses which are checking some specific condition and if it fails return a fail object with a message.

Here is an example code:

public Response sendFirstInquiryEmail(long itemId) {

        Item item = itemService.findById(itemId);

        if (item == null) {
            return Response.fail().withComment("Can't find item");
        }

        if (State.NEW != item.getState()) {
            return Response.fail().withComment("Can't send first inquiry to customer for items not in 'NEW' state");
        }

        if (!item.hasNumber()) {
            return Response.fail().withComment("Can't find ticket in OTRS");
        }

        if (!item.isActive()) {
            return Response.fail().withComment("Item is not active anymore");
        }

        if (!item.isReady()) {
            return Response.fail().withComment("Item is not finalized, first inquiry can be send only for processed items");
        }

        send(item);

        return Response.success();

}

And here is my response:

public class Response {

    private       String  comment;
    private final boolean successful;

    private Response(boolean successful) {
        this.successful = successful;
    }

    public static Response success() {
        return new Response(true);
    }

    public static Response fail() {
        return new Response(false);
    }

    Response withComment(String comment) {
        this.comment = comment;
        return this;
    }

    public String comment() {
        return comment;
    }

    public boolean isSuccessful() {
        return successful;
    }

}

Those conditions are getting more and more, and do not like these bunch of if. Is there a better way to achieve the same? I was thinking if it is possible somehow to map this conditions with errors, so that I can call some validate() which can return the response with a message.

I am going to critique your Response class. Firstly, I think that code should handle the common cases really well, and, hopefully, success happens often.

I would create a constant SUCCESS instance to indicate the success, and all other instances are fails. As a consequence, I would not have the withComment method. How about:

public final class Response {

    public static final Response SUCCESS = new Response();

    private final String  comment;

    private Response() {
        this.comment = null;
    }

    public Response (String comment) {
        this.comment = Objects.requireNonNull(comment, "Fail responses require a non-null comment");
    }

    public String comment() {
        return comment;
    }

    public boolean isSuccessful() {
        return this == SUCCESS;
    }

}

With the above class, any constructed Response is automatically a fail... only the static SUCCESS instance is a success.

Your use code then becomes:

public Response sendFirstInquiryEmail(long itemId) {

    Item item = itemService.findById(itemId);

    if (item == null) {
        return new Response("Can't find item");
    }

    if (State.NEW != item.getState()) {
        return new Response("Can't send first inquiry to customer for items not in 'NEW' state");
    }

    if (!item.hasNumber()) {
        return new Response("Can't find ticket in OTRS");
    }

    if (!item.isActive()) {
        return new Response("Item is not active anymore");
    }

    if (!item.isReady()) {
        return new Response("Item is not finalized, first inquiry can be send only for processed items");
    }

    send(item);

    return Response.SUCCESS;

}

As for all those conditions that are tested in the cascading if blocks? Well, there's not much you can do about that. They are guard conditions. Your only option is to perhaps incorporate the cascade somewhere else, but all that does is shift the logic.

I would live with it. It is clear, flows logically, and so on.


wrapper for a web request call This class is actually a wrapper around HttpWebRequest to issue a POST request with some payloads:

public class SendMessageService
{
    public async Task Send()
    {
        var request = WebRequest.Create("url") as HttpWebRequest;

        request.Method = "POST";
        request.ContentType = "application/json; charset=utf-8";

        request.Headers["authorization"] = "token";

        byte[] byteArray = Encoding.UTF8.GetBytes("{"
                                                + "\"app_id\": \"appId\","
                                                + "\"extra_parameter\": \"value\"}");

        string responseContent = null;

        try
        {
            using (var writer = await request.GetRequestStreamAsync())
            {
                writer.Write(byteArray, 0, byteArray.Length);
            }

            using (var response = await request.GetResponseAsync() as HttpWebResponse)
            {
                using (var reader = new StreamReader(response.GetResponseStream()))
                {
                    responseContent = await reader.ReadToEndAsync();
                }
            }
        }
        catch (WebException ex)
        {
            System.Diagnostics.Debug.WriteLine(ex.Message);
            System.Diagnostics.Debug.WriteLine(new StreamReader(ex.Response.GetResponseStream()).ReadToEnd());
        }

        System.Diagnostics.Debug.WriteLine(responseContent);
    }
}

this class is going to be used in a web environment (ASP.NET Core app) so I registered this service as transient (created each time it is requested):

services.AddTransient<SendMessageService, SendMessageService>();

I would appreciate any improvement suggestions.

Name the method SendAsync since it is meant to be used asynchronously. Since this is in a web app, all awaited calls should chain to .ConfigureAwait(false). Also, since you're async, use await writer.WriteAsync rather than writer.Write. A few more Streams need to be wrapped in using statements. Don't swallow exceptions -- Logging them is all right. Method does not seem to access any instance data, so it could be static and therefore the class it is in can be as well. Here is a rewrite:

    public static async Task SendAsync()
    {
        var request = WebRequest.Create("url") as HttpWebRequest;

        if (request == null)
        {
            return;
        }

        request.Method = "POST";
        request.ContentType = "application/json; charset=utf-8";

        request.Headers["authorization"] = "token";

        var byteArray = Encoding.UTF8.GetBytes("{"
                                                  + "\"app_id\": \"appId\","
                                                  + "\"extra_parameter\": \"value\"}");

        string responseContent = null;

        try
        {
            using (var writer = await request.GetRequestStreamAsync().ConfigureAwait(false))
            {
                await writer.WriteAsync(byteArray, 0, byteArray.Length).ConfigureAwait(false);
            }

            using (var response = await request.GetResponseAsync().ConfigureAwait(false) as HttpWebResponse)
            {
                if (response != null)
                {
                    using (var responseStream = response.GetResponseStream())
                    {
                        if (responseStream != null)
                        {
                            using (var reader = new StreamReader(responseStream))
                            {
                                responseContent = await reader.ReadToEndAsync().ConfigureAwait(false);
                            }
                        }
                    }
                }
            }
        }
        catch (WebException ex)
        {
            System.Diagnostics.Debug.WriteLine(ex.Message);
            if (ex.Response != null)
            {
                using (var responseStream = ex.Response.GetResponseStream())
                {
                    if (responseStream != null)
                    {
                        using (var reader = new StreamReader(responseStream))
                        {
                            System.Diagnostics.Debug.WriteLine(await reader.ReadToEndAsync().ConfigureAwait(false));
                        }
                    }
                }
            }

            throw;
        }

        System.Diagnostics.Debug.WriteLine(responseContent ?? string.Empty);
    }


CSV-simplified: A small open source project I'm working on a small open source project called CSV-simplified for a course I'm taking (Bloc), and I'm looking for some feedback and potential collaborators. I don't actually know an programmers, so I'm hoping someone here may be interested in helping out.

The basic idea of CSV-simplified is a browser app capable of reading a CSV file and displaying it on the page. I would like to expand upon this by creating an intuitive user interface to allow the user to perform searches and queries on the uploaded file. I'm currently using JavaScript, jQuery, and PapaParse to just display an uploaded file.

I'm looking for any help, suggestions, or feedback on improving and building upon this program. The first thing I'd like to see improved is how the file is displayed, I think editing the JS and adding some CSS could really spruce it up.

Here's the GitHub repo of what's done so far. Here's a Codepen that shows the basic logic that's currently working on the app.

HTML

<input type="file" name="File Upload" id="txtFileUpload" accept=".csv" />

JavaScript

document.getElementById('txtFileUpload').addEventListener('change', upload, false);

function upload(evt) {

    var data = null;
    var file = evt.target.files[0];
    var reader = new FileReader();
    reader.readAsText(file);
    reader.onload = function(event) {
        var csvData = event.target.result;
        var data = Papa.parse(csvData, {header : false});
        console.log(data.data);
        var arrayLength = data.data.length;
        console.log(arrayLength);
        for (var i = 0; i < arrayLength; i++) {
            console.log(data.data[i]);
            $("#header").append("
" +
            JSON.stringify(data.data[i]) + "
");
        }
    };
    reader.onerror = function() {
        alert('Unable to read ' + file.fileName);
    };
}

Review

Hi and welcome to code review.

To help out I have numbered the lines of your code for reference.

I will ignore the console.log calls as they represent debugging code.

Line 3 Bad function name. You are not uploading a file, you are loading the file and displaying it. That is two separate tasks and should be handled by two separate functions.

It is important that functions do one thing (their role) as describe in the function name. Two functions loadCSV and displayCSV and the directing of what to load and where to display is upto the event listing for the CSV file.
Line 5. The variable 'data` is not used. You declare it again in the onload line 11.

You don't need to set a value to null if you have nothing to put in it yet. The default is undefined.
Lined 6 & 7 The variables file and reader should be constants as they dont change.
Line 10 the variable csvData should be a const but you only use it once on  line 11. Would have been better to add event.target.result on line 11
Line 11 'data' should be a const. You are storing the wrong thing in it. If you look at the rest of the code lines 12,13,16,18 you use data.data. If you assigned the second property data to data then you would not have to type data.data each time.

Thus the line becomes const data = Papa.parse(event.target.result, {header : false}).data;
Line 13  const for arrayLength but you don't need it. Tthe 'console.log' on line 12 will have the length and the for loop (line 15) is the only other place you use it. But next point means you don't need it at all.
Line 15 First the i should be a block scoped variable using let and data.length for the length eg for(let i = 0; i < data.length; i++) {

But rather than use a for(;;) loop you can use a for of loop. Thus line 15 becomes for(const item of data) { where item is each item data. It's a const as it is a new variable each iteration.
Lines 17,18 is a very poorly written. First line 18 should be indented one tab as it is a continuation of line 17.

The query $("#header") is very slow and you get the same element each time. Before the loop you should have found the element and stored it in a const saving heaps of time.

You append HTML to the page which means the browser must parse it, then do a reflow, for every item, so slow. It can be done much faster. See examples
Lines 21,22,23 (and line 9) There is a shorthand way of writing functions, called arrow functions. They help reduce code noise making the code smaller and easier to read.

They differ a little from standard functions but for now you can just change lines 21-23 to reader.onerror = ()=> alert('Unable to read ' + file.fileName);

 1    document.getElementById('txtFileUpload').addEventListener('change', upload, false);
 2
 3    function upload(evt) {
 4
 5        var data = null;
 6        var file = evt.target.files[0];
 7        var reader = new FileReader();
 8        reader.readAsText(file);
 9        reader.onload = function(event) {
10            var csvData = event.target.result;
11            var data = Papa.parse(csvData, {header : false});
12            console.log(data.data);
13            var arrayLength = data.data.length;
14            console.log(arrayLength);
15            for (var i = 0; i < arrayLength; i++) {
16                console.log(data.data[i]);
17                $("#header").append("<li>" +
18                JSON.stringify(data.data[i]) + "</li>");
19            }
20        };
21        reader.onerror = function() {
22            alert('Unable to read ' + file.fileName);
23        };
24    }

Rewrite

The first version is the simplest one (incase you have not used promises before) but not perfect. The role of loadCSV is not quite just loading as it passes it on to displayCSV. But would require callbacks to do properly so it will do as is.

Both example don't need jQuery

const uploadInputEl = document.getElementById('txtFileUpload')
uploadInputEl.addEventListener('change',event => { loadCSV(event.target.files[0]) });

// The following function would normally be part of some help utilities like jQuery
const createTag = type => document.createElement(type);

function loadCSV(file) {
    const reader = new FileReader();
    reader.readAsText(file);
    reader.onload = e => displayCSV(Papa.parse(e.target.result, {header : false}).data);
}

function displayCSV(data) {
    const list = createTag("ul");
    for (const item of data) {
        list.appendChild(
            object.assign(
                createTag("li"), {textContent : JSON.stringify(item)}
        ));
    }
    document.getElementById("CSVContainer").appendChild(list);
}

The next uses promises to help keep the function roles succinct. LoadCSV only loads the data, and displayCSV creates the displayable content, that goes back to the event that started it all to place where it is wanted.

const uploadInput = document.getElementById('txtFileUpload');
const CSVContainer = document.getElementById("CSVContainer");

uploadInput.addEventListener("change",fileListener);

function fileListener(event) {
    loadCSV(event.target.files[0])
        .then(data => displayCSV(data)
            .then(list => CSVContainer.appendChild(list))
        );
}

function loadCSV(file) {
    return new Promise(loaded => {
        const reader = new FileReader();
        reader.readAsText(file);
        reader.onload = e => loaded(Papa.parse(e.target.result, {header : false}).data);
    });
}

function displayCSV(data) {
    const list = tag("ul");
    for (const item of data) {
        list.appendChild(tagText("li", JSON.stringify(item)));
    }
    return Promise.resolve(list);
}

// The next two would be part of a utils lib  and beyond the scope of the answer
// They do not represent well written code.
const tag = type => document.createElement(type);
const tagText = (type, text) => (type = tag(type), type.textContent = text, type);


Logging errors and debug messages I'm currently working on a small project that uses Java to handle a socket-based MMO browser game. I'm still at the beginning of the project and I'm currently writing a class that logs errors and debug messages to the console and writes these to the appropriate file, based on the error.

I'm wondering if this code is clean, efficient, readable, and amendable in any way shape or form.

package nl.finicky.lemonade.core;
import nl.finicky.lemonade.Lemonade;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.io.PrintWriter;

public class Console {
    public static final String ANSI_RESET = "\u001B[0m";
    public static final String ANSI_BRIGHT = "\u001B[1m";
    public static final String ANSI_BLACK = "\u001B[30m";
    public static final String ANSI_RED = "\u001B[31m";
    public static final String ANSI_GREEN = "\u001B[32m";
    public static final String ANSI_YELLOW = "\u001B[33m";
    public static final String ANSI_BLUE = "\u001B[34m";
    public static final String ANSI_PURPLE = "\u001B[35m";
    public static final String ANSI_CYAN = "\u001B[36m";
    public static final String ANSI_WHITE = "\u001B[37m";
    private static File debug = new File("logs//debug.txt");
    private static File packets = new File("logs//packets.txt");
    private static File errors = new File("logs//errors.txt");

    private static PrintWriter debugWriter;
    private static PrintWriter packetWriter;
    private static PrintWriter errorWriter;

    public Console() {
        try {
            if (!debug.exists()) {
                debug.createNewFile();
            }

            if (!packets.exists()) {
                packets.createNewFile();
            }

            if (!errors.exists()) {
                errors.createNewFile();
            }

            debugWriter = new PrintWriter(new FileWriter(debug));
            packetWriter = new PrintWriter(new FileWriter(packets));
            errorWriter = new PrintWriter(new FileWriter(errors));
        } catch (IOException e) {
            e.printStackTrace();
        }

    }

    public void start(Object message) {
        if (Lemonade.getConfiguration().getBool("logging.debug")) {
            debugWriter.write(Lemonade.getDate() + message.toString() + "\r\n");
        }

        System.out.println(ANSI_GREEN + ANSI_BRIGHT + "[START] " + message.toString() + ANSI_RESET);
    }
    public void user(Object message) {
        if (Lemonade.getConfiguration().getBool("logging.debug")) {
            debugWriter.write(Lemonade.getDate() + message.toString() + "\r\n");
        }

        System.out.println(ANSI_BLUE + "[USER] " + message.toString() + ANSI_RESET);
    }

    public void shutdown(Object message) {
        if (Lemonade.getConfiguration().getBool("logging.debug")) {
            debugWriter.write(Lemonade.getDate() + message.toString() + "\r\n");
        }
        System.out.println(ANSI_YELLOW + ANSI_BRIGHT + "[SHUTDOWN] " + message.toString() + ANSI_RESET);
    }

    public synchronized void error(Object message) {
        errorWriter.write(Lemonade.getDate() + message.toString() + "\r\n");
        System.out.println(ANSI_RED + "[ERROR] " + message.toString() + ANSI_RESET);
    }

    public synchronized void critical(Object message) {
        errorWriter.write(Lemonade.getDate() + message.toString() + "\r\n");
        System.out.println(ANSI_RED + "[ERROR] " + message.toString() + ANSI_RESET);
    }

    public synchronized void debug(Object message) {
        if (Lemonade.getConfiguration().getBool("logging.debug")) {
            debugWriter.write(Lemonade.getDate() + message.toString() + "\r\n");
        }

        System.out.println(ANSI_PURPLE + "[DEBUG] " + message.toString() + ANSI_RESET);
    }

    public synchronized void packet(Object message)
    {
        System.out.println(ANSI_BLUE + "[PACKET] " + message.toString() + ANSI_RESET);

        if(Lemonade.getConfiguration().getBool("logging.packets"))
        {
            packetWriter.write(Lemonade.getDate() + message.toString() + "\r\n");
        }
    }
}

A synchronized method will block execution of other methods in the class, you better use a seperate thread to store the logsentries to write.

This could be a simple write-only-thread.

.error and .critical are the same, this is violating DRY.


Using serializable to preserve/save objects I'm building one of my first android apps. I met this problem that everytime you change screen orientation, Android destroys the app and builds it up again (I know this is more complex, but this isn't the subject of this post.)

I am wondering if it is good idea to have a Game game variable inside my MainActivity.java that would simply implement Serializable. This way I can do this:

public class MainActivity extends AppCompatActivity {

    private MagicSquareGame game;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);
        game = MagicSquareGame.getEngine();
        initEditTextReferences();
        initButtonReferences();
        updateSumResultsOnScreen();
    }

    (all the other stuff...)

    @Override
    protected void onSaveInstanceState(Bundle outState) {
        outState.putSerializable("game", game);
        Log.i("MSG", "saving Game");
        super.onSaveInstanceState(outState);
    }

    @Override
    protected void onRestoreInstanceState(Bundle savedInstanceState) {
        super.onRestoreInstanceState(savedInstanceState);
        game = (MagicSquareGame) savedInstanceState.getSerializable("game");
        updateSumResultsOnScreen();
        Log.i("MSG", "loading Game");
    }

Is this an okay approach? Code smell? What problems can I meet?

If this is a private project, where you're free to use alpha-staged libraries I'd strongly recommend you take a look at Android Architecture Components. A great talk about the subject was given at the recent Google I/O.

Restructuring your app's architecture and using Android Architecture component - the LifeData and LifecycleObserver in particular will very much ease the problem of having to deal with LifeCycle events (such as screen orientation change).

If you're writing a game, then the Component-based Architecture may be interesting for your purposes. You'll very likely come across it when investigating architectures suitable for Games (render loops etc).

The general idea is:

Separation of concerns and improving testability. Ideally you should be able to unit-test your entire game with locally running Java tests and no UI necessary whatsoever. This however requires you to separate your Game instance from your activity - something I'd highly recommend either way.
Making your game modular and plugging it together in the form of components (this architecture style is a matter of preference but lends itself quite well to games)

Let me know, if you want me to elaborate on any of the points I mentioned.


Too many nested Types? I have the following Scala code:

  def getLists(token: String, username: String):Future[Try[Seq[Group]]] = {
    val result:Future[Try[JsValue]] = getAPIResult(token, "https://myapi.com/screen_name=" + username)
    result.map( x => convertFromJsValueToObject(x))
  }

  def getSubscriptions(token: String, username: String):Future[Try[Seq[Group]]] = {...} //same as above

  private def getAPIResult(token: String, apiCall: String):Future[Try[JsValue]] = {
    WS.url(apiCall)
    .withHeaders("Authorization" -> ("Bearer " + token))
    .get().map(response =>
       response.status match {
       case 200 => Success(Json.parse(response.body))
       case _ => Failure(new RuntimeException("Web service call failed: " + response.body))
    })
  }

  private def convertFromJsValueToObject(json: Try[JsValue]): Try[Seq[Group]] = {
    json match {
      case Success(v) => {
        Success(v.as[Seq[Group]])
      }
      case Failure(t) => Failure(t)
      }
  }

and I have a third function like this:

def getAllListsAndSubscriptions(token: String, username: String):Future[Try[Seq[Group]]] = {...}

Which is supposed to concatenate the two Seq collections of Group objects together and return them as per the type signature. I'm really struggling to figure out how to do this though, as the types are so deeply nested.

Have I written my code badly or is there another way to do this?

You should "lift" your functions instead.

For example, instead of defining:

def getSomething(input: V): Future[FinalType] = {
   val intermediate: IntermediaryType = complicatedFunctionReturningFuture(input)
   mappingFunction(intermediate)
}

use

complicatedFunctionReturningFuture(input).map(mappingFunction)

directly in your code.

You can think of map as a method that is lifting a function IntermediaryType => FinalType to a function Future[Intermediary] => Future[FinalType].

If you have a Future[Try[Type]], you can do a "double lifting" (I'm claiming the trademark on that term!):

complicatedFunctionReturningFutureTry(input).map(_.map(mappingFunction))

which lifts the same function IntermediaryType => FinalType to a function Future[Try[Intermediary]] => Future[Try[FinalType]].


SICP - exercise 2.69 - generate a huffman tree from a set of ordered leaves From SICP

  Exercise 2.69: The following procedure takes as its argument a list of
  symbol-frequency pairs (where no symbol appears in more than one pair)
  and generates a Huffman encoding tree according to the Huffman
  algorithm.

(define (generate-huffman-tree pairs)
  (successive-merge
   (make-leaf-set pairs)))

  Make-leaf-set is the procedure given above that transforms the list of
  pairs into an ordered set of leaves. Successive-merge is the procedure
  you must write, using make-code-tree to successively merge the
  smallest-weight elements of the set until there is only one element
  left, which is the desired Huffman tree. (This procedure is slightly
  tricky, but not really complicated. If you find yourself designing a
  complex procedure, then you are almost certainly doing something
  wrong. You can take significant advantage of the fact that we are
  using an ordered set representation.)

make-code-tree creates a tree from two branches, make those branches as left and right branches respectively, combine all symbols of the branches as its own set of symbols, and finally add the weights of the branches as its own weight.
adjoin-set inserts a tree to the set while remaining ordered based on the weight of the items.

Though I am confident my code is correct, I am extremely unsure of my solution. What's bothering me is the hint that it's tricky. It does gave correct results for all tests I did. I have thought of alternative ways of doing this, but the procedures I came up with were very complex. Also, I think this is very slow. Please review my code.

(define (successive-merge leaves)
(if     (= (length leaves) 1)
        (car leaves)
    (successive-merge (adjoin-set (make-code-tree (car leaves) (cadr leaves)) (cddr leaves)))))

How can I make this code better and faster? And are there better ways of doing this?

Instead of using (= (length leaves) 1) to measure the list, consider
just using (null? (cdr leaves)) instead, that way it's a constant
operation.

Otherwise, if you're not using destructive operations (to improve
adjoin-set), this looks as good as it can get.

Also make sure to have the correct indentation (if that wasn't caused by
pasting), e.g.:

(define (successive-merge leaves)
  (if (null? (cdr leaves))
      (car leaves)
      (successive-merge
       (adjoin-set
        (make-code-tree (car leaves)
                        (cadr leaves))
        (cddr leaves)))))

Also take a look at this post maybe, one of the posts regarding the same exercise.


Using QXmlStreamReader to read configuration file of key-value pairs Background

The following XML snippet contains configuration parameters for our application in key-value pair manner. (Despite using INI files for such purpose may be better, I have to use XML files.)

<configuration>
    <parameter id="serial_port_name">ttyS0</parameter>
    <parameter id="serial_port_baud_rate">9600</parameter>
    <parameter id="check_period">1000</parameter>
    <parameter id="locale">hu_HU</parameter>
    <!-- ... -->
</configuration>

I would like to read the configuration parameters and store them inside a QHash<QString, QString> object with minimal overhead, therefore I've chosen the QXmlStreamReader for iterating through XML elements.

Current code

The toConfigurationMap() method of the following class iterates through the elements of XML file and when a <configuration> element is found, starts another iteration on the level of <parameter> elements to parse them.

class ConfigurationReader {
public:
    explicit ConfigurationReader(QFile& input) : reader(&input) {
    }

    QHash<QString, QString> toConfigurationMap() {
        QHash<QString, QString> output;

        for(auto token = reader.tokenType(); !reader.atEnd();
                token = reader.readNext()) {
            if(token != QXmlStreamReader::StartElement ||
                    reader.name() != "configuration")
                continue;

            while(reader.readNextStartElement()) {
                if(reader.name() == "parameter") {
                    QString id = reader.attributes().value("id").toString();
                    output.insert(id, reader.readElementText());
                }
                else {
                    reader.skipCurrentElement();
                }
            }
        }

        return output;
    }

private:
    QXmlStreamReader reader;
};

Goals

I would like to reduce the complexity of the code above, by somehow take apart the nested loops. In order to the toConfigurationMap() method be more cleaner, I would like to communicate to the reader that "it parses the parameters for each configuration elements" through the code.

Case for not using a class

It's not clear to me whether you need a class for what you are doing. Unless you need to be able to construct an instance of ConfigurationReader and pass it around, I would say get rid of the class. You can have a function with the simple interface

QHash<QString, QString> getConfigurationMap(QFile& input);

I would further suggest putting the function in a namespace that is specific to your app.

namespace MyApp
{
   QHash<QString, QString> getConfigurationMap(QFile& input);
}

Use smaller functions

Instead of one large function, divide the code into multiple functions. Think of what you are doing rather than how you are doing it. The whats usually translate to smaller functions and hows translate to loops, conditionals, branches, etc.

Given that, getConfigurationMap can be implemented as:

namespace MyApp
{
   // Implementation detail.
   void getConfigurationMap(QXmlStreamReader& reader,
                            QHash<QString, QString>& map);

   // User facing interface.
   QHash<QString, QString> getConfigurationMap(QFile& input)
   {
      QXmlStreamReader reader(input);
      QHash<QString, QString> map;
      getConfigurationMap(reader, map);
      return map;
   }
}

The second overload (the one that's listed first) can be implemented as:

namespace MyApp
{
   void skipUntilConfiguration(QXmlStreamReader& reader);

   void readParamters(QXmlStreamReader& reader,
                      QHash<QString, QString>& map);

   // Implementation detail.
   void getConfigurationMap(QXmlStreamReader& reader,
                            QHash<QString, QString>& map);
   {
      skipUntilConfiguration(reader);
      readParamters(reader, map):
   }

   // User facing interface.
   QHash<QString, QString> getConfigurationMap(QFile& input)
   {
      QXmlStreamReader reader(input);
      QHash<QString, QString> map;
      getConfigurationMap(reader, map);
      return map;
   }
}

The code can be wrapped up by implementing the final two helper functions.

namespace MyApp
{
   void skipUntilConfiguration(QXmlStreamReader& reader)
   {
      whiile ( !reader.atEnd() )
      {
         if ( reader.tokenType() == QXmlStreamReader::StartElement &&
              reader.name() == "configuration" )
         {
            return;
         }
      }
   }

   void readParamters(QXmlStreamReader& reader,
                      QHash<QString, QString>& map)
   {
      while ( reader.readNextStartElement() ) {
          if ( reader.name() == "parameter" ) {
              QString id = reader.attributes().value("id").toString();
              map.insert(id, reader.readElementText());
          }
          else {
              reader.skipCurrentElement();
          }
      }
   }

   void getConfigurationMap(QXmlStreamReader& reader,
                            QHash<QString, QString>& map)
   {
      skipUntilConfiguration(reader);
      readParamters(reader, map):
   }

   QHash<QString, QString> getConfigurationMap(QFile& input)
   {
      QXmlStreamReader reader(input);
      QHash<QString, QString> map;
      getConfigurationMap(reader, map);
      return map;
   }
}

The above code assumes that there is only one "configuration" element in your XML file. If that assumption is not correct, the functions need to be tweaked a bit.


Is Refactoring with different class casting and property calling possible ? Can i refactor these code ?

The diffrent between these block of code are.

They are using different casting type.
They call different property

    var objBillOfMaterialRow = (MISheetDataSet.BillOfMaterialRow)pObjDataset.Tables["BillOfMaterial"].Rows[0];
    if (objBillOfMaterialRow.mi1_BillOfMaterial == ShowReport.False)
    {
        pObjDataset.Tables["BillOfMaterial"].Clear();
    }

    var objLayerBuildUpRow= (MISheetDataSet.LayerRow)pObjDataset.Tables["LayerBuildUp"].Rows[0];
    if (objLayerBuildUpRow.mi1_LayerBuildUp == ShowReport.False)
    {
        pObjDataset.Tables["LayerBuildUp"].Clear();
    }

    var objConstructionRow = (MISheetDataSet.ConstructionRow)pObjDataset.Tables["Construction"].Rows[0];
    if (objConstructionRow.mi2_Construction == ShowReport.False)
    {
        pObjDataset.Tables["Construction"].Clear();
    }

This should work if you don't want to change anything in your classes:

var rows = new Dictionary<string, Func<DataRow, ShowReport>>
       {
          {"BillOfMaterial", dr => ((MISheetDataSet.BillOfMaterialRow)dr).mi1_BillOfMaterial},
          {"LayerBuildUp", dr => ((MISheetDataSet.LayerRow)dr).mi1_LayerBuildUp},
          {"Construction", dr => ((MISheetDataSet.ConstructionRow)dr).mi2_Construction},
       };

foreach (var row in rows)
{
    var dataRow = pObjDataset.Tables[row.Key].Rows[0];
    var showReportExtractor = row.Value;
    var showReport = showReportExtractor(dataRow);
    if (showReport == ShowReport.False)
        pObjDataset.Tables[row.Key].Clear();
}

(Don't forget to give meaningful names to variables)

But I would consider refactoring classes to allow more straightforward extraction of these mi1_BillOfMaterial, mi1_LayerBuildUp, mi2_Construction members. You have three classes here, each of them has ShowReport property but with different name. The idea is to extract a base class which will have this property, in this case we won't need to know the exact type of the row:

public abstract class RowBase
{
    public abstract ShowReport ShowReport { get; }
}

public class BillOfMaterialRow : RowBase
{
    public override ShowReport ShowReport { get { return mi1_BillOfMaterial; } }
}

// Two other classes implemented in the same way

And then your method will be as simple as:

var rows = new []
       {
          "BillOfMaterial",
          "LayerBuildUp",
          "Construction",
       };

foreach (var tableName in rows)
{
    var row = (RowBase)pObjDataset.Tables[tableName].Rows[0];
    if (row.ShowReport == ShowReport.False)
        pObjDataset.Tables[tableName].Clear();
}


Longest way for all vertex how can I optimalize this code?

#include<bits/stdc++.h>
using namespace std;

class Graph {
    int V;
    list<int> *adj;
    void DFSUtil(int v, bool visited[]);
public:
    Graph(int V);
    void addEdge(int v, int w);
    void DFS(int v);
};

Graph::Graph(int V) {
    this->V = V;
    adj = new list<int>[V];
}

void Graph::addEdge(int v, int w) {
    adj[v].push_back(w);
}

void Graph::DFSUtil(int v, bool visited[]) {
    visited[v] = true;
    cout << v << " ";

    list<int>::iterator i;
    for (i = adj[v].begin(); i != adj[v].end(); ++i)
        if (!visited[*i])
            DFSUtil(*i, visited);
}

void Graph::DFS(int v) {
    bool *visited = new bool[V];
    for (int i = 0; i < V; i++)
        visited[i] = false;
    DFSUtil(v, visited);
}

int main() {
    Graph g(4);
    g.addEdge(0, 1);
    g.addEdge(0, 2);
    g.addEdge(1, 2);
    g.addEdge(2, 0);
    g.addEdge(2, 3);
    g.addEdge(3, 3);

    for (int i = 0; i < 4; i++) {
        g.DFS(i);
        puts("");
    }
    return 0;
}

I mean, for now I calculate path for each vertex all over again. Well, it's very inefficient for many vertex, so my answer is: How can I rebuild this code such that I don't must calculate from zero

Few habits to learn early in life.

passing big arguments by references. ( bool& visited[] )
deleting unused memory. (delete visited)
proper indentation always makes it better.

Here is the complete code.

#include<bits/stdc++.h>
using namespace std;

class Graph
{
    int V;
    list<int> *adj;
    void DFSUtil(int v, bool& visited[]);
    public:
    Graph(int V);
    void addEdge(int v, int w);
    void DFS(int v);
};

Graph::Graph(int V)
{
    this->V = V;
    adj = new list<int>[V];
}

void Graph::addEdge(int v, int w)
{
    adj[v].push_back(w);
}

void Graph::DFSUtil(int v, bool& visited[])
{
    visited[v] = true;
    cout << v << " ";
    list<int>::iterator i;
    for (i = adj[v].begin(); i != adj[v].end(); ++i)
        if (!visited[*i])
            DFSUtil(*i, visited);
}

void Graph::DFS(int v)
{
    bool *visited = new bool[V];
    for (int i = 0; i < V; i++)
        visited[i] = false;
    DFSUtil(v, visited);
    delete visited; // avoids memory leaks
}

int main()
{
    Graph g(4);
    g.addEdge(0, 1);
    g.addEdge(0, 2);
    g.addEdge(1, 2);
    g.addEdge(2, 0);
    g.addEdge(2, 3);
    g.addEdge(3, 3);

    for(int i = 0; i < 4; i++) {
        g.DFS(i);
        puts("");
    }
    return 0;
}

Apart from these your code is very optimized possible.


Processing Batch Job Background
I have a script that either takes a text file that contains a list of player_ids or not. If the user provides a player_id list i ensure that all the ids are part of the tranche date they provided and then run the process job for all the player ids in the list.
If the user has not provided a list of players to run the jobs on we run the process job on all the players that are part of the tranche date.
Code
import argparse
from nba import release_checks, s3, shell
from typing import List
from nba.process_jobs_off_sqs import batch
from nba.process_jobs_off_sqs.exceptions import BatchTimeoutException

def _get_all_player_id(tranche_date: str) -> List[str]:
    return s3.download_str_key(
        s3.S3Path(
            "player-datasets",
            f"ENVIRONMENTS/staging/{tranche_date}/all_player_ids_in_delivery.txt",
        )
    ).splitlines()

def _is_player_id_list_valid(
    provided_player_id_list: List[str], all_player_id_list: List[str]
) -> bool:
    for player_id in provided_player_id_list:
        if player_id not in all_player_id_list:
            raise Exception(
                f"The following player id: {player_id} in the text file is not part of the tranche you provided."
            )
    return True

def _launch_process_job(list_of_player_ids: List[str]) -> None:
    jobs = [
        {"player_id": player_id, "job_id": player_id}
        for player_id in list_of_player_ids
    ]
    try:
        batch.launch_and_await_batch("automated_player_selection", jobs)
    except BatchTimeoutException:
        print(f"Jobs are known to fail due data missingness.")

def _run_mentality_check(nba_folder_name: List[str]) -> None:
    try:
        release_checks.mentality_check(
            nba_folder_name
        )
    except Exception as e:
        print(e)

def parse_args():
    parser = argparse.ArgumentParser()
    parser.add_argument("--nba-folder-name", required=True)
    parser.add_argument("--tranche-date", required=True)
    parser.add_argument("--list-of-player-id-file-path")
    return parser.parse_args()

if __name__ == "__main__":
    args = parse_args()
    all_player_id_list = _get_all_player_id(args.tranche_date)
    if args.list_of_player_id_file_path:
        provided_player_id_list = shell.lines_from_file(
            args.list_of_player_id_file_path
        )
        if _is_player_id_list_valid(provided_player_id_list, all_player_id_list):
            _launch_process_job(provided_player_id_list)
    else:
        _launch_process_job(provided_player_id_list)
    _run_mentality_check(args.nba_folder_name)

I am mostly looking for a code review for better python usage as i am new to python. I gave a vague background as i am less concerned on getting code review on the actual algorithm but more so about my code and how to make it more pythonic and clean.

Validation
This function _is_player_id_list_valid is stuck between two useful concepts - validating and returning bool, and throwing-or-not. Don't attempt to do a half-measure of both. Given its current name, it would be less surprising to do
for player_id in provided_player_id_list:
    if player_id not in all_player_id_list:
        return False
return True

If you want to keep the exception, then

Delete the return
Change the return type to None
Use a more specific type than Exception
Rename the method to something like check_player_id_list.

For this method you should also get rid of the loop, cast the lists to sets, use set intersection, and then base your error message off of all of the missing elements instead of just the first.
Late serialization
Validation should be done on tranche_date. The sanest way to do this is expect a date of a specific format (which you probably already do, though you haven't shown it); parse it into a real datetime (or perhaps date), and then re-serialize it in _get_all_player_id. Its representation should only be str at the extreme edges of your program - in your argument parsing, and in your S3 call. In the middle it should be a real date type.
Error messages
except BatchTimeoutException:
    print(f"Jobs are known to fail due data missingness.")

is... a little strange. You could print this and it would still be valid even if there were no timeout. Instead perhaps consider
    print('Batch timed out. Data may be missing.')

Note that this does not need to be an f-string. Also, what data?


Prompting the user for employment information I have created a simple program in which the user is prompted for various details about their employment. In my Employee.java file, I have all the methods.

Employee.java:

import javax.swing.JOptionPane;

public class Employee {

    private static String name;
    private static String jobTitle;
    private static int age;
    private static int salary;
    private static int taxAmount;
    private static int salaryPostTax;

    public static int getSalary() {
        try {
            String inputSalary = JOptionPane.showInputDialog("Enter your salary:");
            salary = Integer.parseInt(inputSalary);

        } catch (Exception ex) {
            JOptionPane.showMessageDialog(null, "ERROR! That's NOT a numeric value.");
        }
        return salary;
    }

    public static String getName() {
        name = JOptionPane.showInputDialog("Enter your name:");
        return name;
    }

    public static String getJobTitle() {
        jobTitle = JOptionPane.showInputDialog("Enter your Job Title:");
        return jobTitle;
    }

    public static int getAge() {
        try {
            String inputAge = JOptionPane.showInputDialog("Enter your age:");
            age = Integer.parseInt(inputAge);

        } catch (Exception ex) {
            JOptionPane.showMessageDialog(null, "ERROR! That's NOT a numeric value.");
        }
        return age;
    }

    public static int calcTaxAmount () {

        if (salary < 10_000)
            taxAmount = 0;

        if ((salary >= 10_000) && (salary < 30_000))
            taxAmount = 28;

        if ((salary >= 30_000) && (salary < 80_000))
            taxAmount = 38;

        if (salary >= 80_000)
            taxAmount = 48;

        return taxAmount;
    }

    public static int calcSalary () {
        salaryPostTax = salary - taxAmount;

        return salaryPostTax;
    }

    public static void showPayment() {
        if (inputValid() == true) {

            JOptionPane.showMessageDialog(null, "Pay to the order of " + name + " (" + jobTitle +
                    ") the sum of £"+ salaryPostTax);
        } else {
            JOptionPane.showMessageDialog(null, "ERROR! Please fill in ALL the required fields correclty.");
        }
    }

    public static boolean inputValid() {
        if (
                (name != null) &&
                (jobTitle != null) &&
                (!(salary <= 0)) &&
                (!(salaryPostTax <= 0)) &&
                (!(age <= 0))) {
            return true;

        } else {
            return false;
        }
    }
}

I have a file (UseIt.java) in which I use the class. My question though, is: Is it a good programming practice to either:

1. Put all your methods into one method, and simply call that one method rather than calling them all individually, which becomes tiresome?

Employee.java: (Example 1)

//Collate ALL Employee details into ONE method
public static void getAllDetails() {
    Employee.getName();
    Employee.getAge();
    Employee.getJobTitle();
    Employee.getSalary();
    Employee.calcTaxAmount();
    Employee.calcSalary();
    Employee.showPayment();
}

UseIt.java: (Example 1)

public class UseIt {

    static Employee foo = new Employee();

    public static void main(String[] args) {
        foo.getAllDetails(); //Calling ONE method, rather than all of them.
    }
}

2. Put all the methods into the constructor, so that the methods run automatically upon the creation of the instance?

Employee.java (Example 2)

//Collate ALL Employee details into the constructor
public Employee() {
    Employee.getName();
    Employee.getAge();
    Employee.getJobTitle();
    Employee.getSalary();
    Employee.calcTaxAmount();
    Employee.calcSalary();
    Employee.showPayment();
}

UseIt.java (Example 2)

public class UseIt {

    public static void main(String[] args) {
        Employee foo = new Employee();
    }
}

Wrote this on SO so excuse any compilation errors.

As a novice there is a lot to learn, I would start by understanding the SOLID Principles and also looking out for Top 10 errors Java programmers make.

The first thing you want to do is get away from the static main(String[] args) method that starts your program. You do not want to use static methods. Therefore you need instantiate your system (MySystem) so that you are in the world of objects.

You want to keep your responsibilities separate.

Always prefer immutability.

Here I have an immutable Employee object (it cannot be changed once instantiated). From the main method our DetailsRetriever will get the input from the user, this uses the DetailsValidator to ensure the input is correct. If all the details are correct a Factory creates our employee object.

public class UseIt {

    public static void main(String[] args) {
        MySystem foo = new MySystem(new DetailsRetriever(new DetailsValidator()), new EmployeeFactory())
        foo.start();
    }
}

public class MySystem {

   private final DetailsRetreiver detailsRetriever;
   private final EmployeeFactory employeeFactory;

   public MySystem(DetailsRetreiver detailsRetriever, EmployeeFactory employeeFactory) {
         this.detailsRetriever = detailsRetriever;
         this.employeeFactory = employeeFactory;
   }

   public void start() {
         String name = detailsRetriever.retrieveName();
         String jobTitle = detailsRetriever.retrieveJobTitle();
         ..etc

         Employee employee = employeeFactory.create(name, jobTitle, ..etc);
   }

}

public class Employee {

    private final String name;
    private final String jobTitle;
    private final int age;
    private final int salary;

    public Employee(String name, String jobTitle, int age, int salary) {
        this.name = name;
        this.jobTitle = jobTitle;
        this.age = age;
        this.salary = salary;
    }

   public int calcTaxAmount() {
    int calculatedTax;
    if (salary < 10_000)
        calculatedTax = 0;

    if ((salary >= 10_000) && (salary < 30_000))
        calculatedTax = 28;

    if ((salary >= 30_000) && (salary < 80_000))
        calculatedTax = 38;

    if (salary >= 80_000)
        calculatedTax = 48;

    return taxAmount;
}

   public int calcSalary() {
    int calculatedSalaryPostTax = salary - calcTaxAmount();

    return calculatedSalaryPostTax;
   }

}

public class DetailsRetreiver {

    private final DetailsValidator detailsValidator;

    public DetailsRetreiver(DetailsValidator detailsValidator) {
      this.detailsValidator - detailsValidator;
    }

    public String getName() {
        String name = JOptionPane.showInputDialog("Enter your name:");
        if(detailsValidator.validateName(name)) {
          return name;
        } else {
           return getName(); // or throw an exception
        }
    }

    // etc ...

}

public class DetailsValidator {

     public boolean validateName(String input) {
          // add more validation as you need
          return !"".equals(input);
     }

     // etc
}

public class EmployeeFactory {

      public Employee create(String name, String jobTitle, int age, ... etc) {
           return new Employee(name, jobTitle, age .. etc);
      }

}


BFS for creating a queue without repetitions and with loops in the graph I have a pipeline with loops of filters and one input filter.  The rest are splitters, transform and output.

I would like my code to go over the filters and push them into a queue (order is important).  However, if I have a loop, I only want to count the end of the loop once.

Here is my working code.  Please review it.

queue<string> CEngineInternal::GetFiltersListToRun()
{
    queue<string> filtersBFS;
    queue<string> filtersToRun;
    set<string> alreadyFoundFilters;

    // add input filters to the list
    for(BaseFilterMap::const_iterator it = m_Filters.begin(); it != m_Filters.end(); it++)
    {
        string FilterName = (*it).first;
        const CBaseFilter *filter = (*it).second;
        if(filter->IsInputFilter())
        {
            //push the input filter
            filtersBFS.push(FilterName);
            filtersToRun.push(FilterName);
            alreadyFoundFilters.insert(FilterName);
        }
    }

    while (!filtersBFS.empty())
    {
        string filterName = filtersBFS.front();
        filtersBFS.pop();

        //get the OutputPin connections for the next filter
        FilterConnection* filterConnection = m_Connections[filterName];
        ASSERT(filterConnection != NULL);
        PinConnectionVector& outPinConnections = filterConnection->GetOutputPinConnections();

        if (outPinConnections.size() != 0)
        {
            // find all the output pin connections
            for (size_t i = 0; i < outPinConnections.size(); ++i)
            {
                //add connected filters id's to the list
                const std::string& oFilterName = outPinConnections[i].ConnectedFilterName();

                if (oFilterName.empty())
                {
                    continue;
                }

                if (alreadyFoundFilters.find(oFilterName) != alreadyFoundFilters.end())
                {
                    continue;
                }
                else
                {
                    filtersBFS.push(oFilterName);
                    filtersToRun.push(oFilterName);
                    alreadyFoundFilters.insert(oFilterName);
                }
            }
        }
    }

    return filtersToRun;
}

As far as I can tell, this code seems pretty easy to follow, especially with the comments.  Here are several things that stood out to me:

Since you haven't provided your own queue implementation, I assume you're using std::queue.  If so, remove using namespace std and use std:: where necessary.
This entire method should be const.  You're modifying three local containers and returning one, while not modifying any class members.  By making this const, data members will stay immutable (modifications will cause compiler errors) and the reader will be aware of this.

queue<string> CEngineInternal::GetFiltersListToRun() const

filtersBFS is not a very descriptive name; we know this program is about BFS.  It's also not the local queue that is returned, so it is unclear what this is used for within the function.
Your functions start with a capital letter, which is not proper naming in C++.  Only user-defined types should be named this way.  Your variables (except for FilterName) are named correctly, however.  Both variables and functions should start with a lowercase letter.
For this line:

for(BaseFilterMap::const_iterator it = m_Filters.begin(); it != m_Filters.end(); it++)

It may be more readable to declare the iterator type before the loop.

In addition, it may be better to use pre-increment since you're not dealing with basic types.  This could also potentially avoid an extra copy.

BaseFilterMap::const_iterator it;

for (it = m_Filters.begin(); it != m_Filters.end(); ++it) {}

However, if you're using C++11, consider auto instead of your current iterator.  The compiler will determine the correct type, and it's more readable.  You may also keep it inside the loop statement.

for (auto it = m_Filters.begin(); it != m_Filters.end(); ++it) {}

Better yet, if you're using C++11, consider a range-based for-loop:

for (auto& it : m_Filters) {}

This:

if (outPinConnections.size() != 0)

should instead use !empty():

if (!outPinConnections.empty())

For here:

for (size_t i = 0; i < outPinConnections.size(); ++i) {}

If this is the size type returned by size(), make it std::size_t since this is C++.  Otherwise, make sure you're using the correct size type to avoid potential loss-of-data warnings.
These:

(*it).first;
(*it).second;

should use the more readable -> operator:

it->first;
it->second;

You say:

  If I have a loop, I only want to count the end of the loop once.

But I don't see that anywhere in the code, plus there are multiple loops used here.  If you're still receiving the correct output and no errors associated with them, then they should be okay.


PHP Instant Message API I am creating a chat API for Android developers, but it's slow -- the chat app is getting messages a little bit late.

How can I improve the performance of my code?

<?php
require_once('config.php');
$sender_id = $_GET['sender_id'];
$receive_id = $_GET['receive_id'];

$response = array();

if($sender_id && $receive_id)
{

        $results = mysql_query("select * from cpo_chat_system where (sender_id = '$sender_id' AND receive_id = '$receive_id') union select * from cpo_chat_system where (sender_id = '$receive_id' AND receive_id = '$sender_id') ORDER BY date ASC, time ASC");
        $total_record = mysql_num_rows($results);

        if($total_record > 0)
        {
            $response['success'] = '1';
            while($row = mysql_fetch_assoc($results))
            {
                $response['user'][] = $row;
            }
        }
        else
        {
            $response['success'] = '0';
            $response['user']['error'] = "1";
            $response['user']['error_msg'] = "Not Data Found";
        }
}

echo json_encode($response);

?>

Before worrying about performance, you should worry about security. You are vulnerable to SQL injection. Never put any variables directly into queries, use prepared statements instead.

Other than that:

mysql_ is deprecated for quite a while, and it will be removed in future PHP versions. You really shouldn't write code using it, use PDO or mysqli_ instead (and as said, with prepared statements).
upper-case all SQL keywords in your queries to improve readability.
your indentation is inconsistent.
your query seems a bit off. Do you really need union? Shouldn't something like WHERE (sender_id = '$sender_id' AND receive_id = '$receive_id') OR (sender_id = '$receive_id' AND receive_id = '$sender_id') work?
I would change the date and time columns to a datetime column, it should speed up your query a bit.
receive should be receiver to fit in with sender.


Grabs 10 bytes and converts it into a hex formatted string I'm working on some code right now where the goal is to take in a byte stream and grab the data from the body of a message. The data is 10 bytes long and is meant to be translated into hex. Specifically, the 10 bytes coming in look like 01000200030004000500 or 01010201030104010501 (with implied 0x in front of each).

The code does this, but in my opinion it's not very pretty, so I was hoping to get help in improving both my own code and learning about a more effective way of grabbing an "uneven" amount of bytes (i.e. not the size of a data type for an easy memcpy). I've seen suggestions for using an array, but I wasn't sure if memcopying or endianness changes would complicate its use.

case MyMessage:
{
  std::string s;
  unsigned int i,j;
  // bytes are of type unsigned char*
  // each hex value is 4 bits
  memcpy(&i, bytes, sizeof(i)); // Bytes 0, 1, 2, 3
  i = ntohl(i); // 01000200 or 01010201

  memcpy(&j, bytes + 4, sizeof(j)); // Bytes 4, 5, 6, 7
  j = ntohl(j); // 03000400 or 03010401

  unsigned short k;
  memcpy(&k, bytes + 8, sizeof(k)); // Bytes 8, 9
  k = ntohs(k); // 0500 or 0501

  std::ostringstream iS, jS, kS;
  iS << std::hex << (i);
  s += "0" + iS.str(); // Have to shove "0" at front when dropped

  jS << std::hex << (j);
  s += "0" + jS.str();

  kS << std::hex << (k);
  s += "0" + kS.str();

  // In my actual code, these strings are constants
  if (s == "01000200030004000500")
  {
      s = "Off";
  }

  else if (s == "01010201030104010501")
  {
      s = "On";
  }

  else
  {
      s = "ERROR";
  }

  return s;
}

It looks like you overcomplicate things. I don't see why do you need to build the string. You only use it for comparison (and immediately kill it after). Comparing integers is much simpler, especially that your bytes follow nice structure. Inspect bytes one by one, along the lines of (I might be getting even/odd bytes wrong, follow their wire order):

    if ((bytes[1] != 0) && (bytes[1] != 1)) {
        return "Error";
    }

    for (int i = 0; i < 10; i += 2) {
        if (bytes[i] != i) {
            return "Error";
        }
        if (bytes[i + 1] != bytes[1]) {
            return "Error";
        }
    }

    return bytes[1] == 0? "Off": "On";

If the message structure is coincidental, and you don't have such an algorithmic approach handy, I recommend to format expected messages as byte arrays, e.g.

    char off_msg[] = {0x01, 0x00, 0x02, 0x00, 0x03, 0x00, 0x04, 0x00, 0x05, 0x00};

and memcmp them against bytes. Again, follow the wire byte order.


Removing duplicates from an unsorted linked list Does the code below look correct and efficient for removing duplicates from an unsorted linked list without using extra memory?

An object of type Node denotes a LL node.

void removeDuplicates()
{
  Node current = head;
  while(current!=null)
  {
    Node prev = current;
    Node next = current.next;
    while(next!=null)
    {
      if(current.equals(next))
      {
         prev.next = next.next;
      }
      else
      {
         prev = next;
      }
      next = next.next;
    }
      current = current.next;
   }
}

This review is two-fold: once, in case you have to uphold your restriction and then what changes if you allow extra space.

Review

Comments: they are simply missing. You need at least some javadoc, and for non-trivial methods like this some more never hurts. Oh, and skip the usual excuses about how this code is somehow special and doesn't need comments.
Clarity and Intent: if (current.equals(head)) reads like you are comparing the actual nodes, while duplicity is a matter of equal node values. You may have overwritten the equals method accordingly (I assume so for the sake of correctness), but I would prefer to clearly portray your original intent here by comparing the actual values.
Efficiency and readability: As the answer by zdd points out you could save one pointer, but that's kind of pointless. I prefer your version as its much clearer to a reader what current, prev and next are. However,efficiency is not going to get below O(n^2) with this approach.

Efficiency Improvement

You said that you need a solution "without using extra memory". What you really meant of course is a solution "with only a constant amount of additional memory" (your pointers require memory as well after all). What you did not require, however, is that this operation has to leave the list elements' ordering intact. Therefore, a faster approach in O(n log(n)) would be to first sort the list (in-place to satisfy the memory requirement), then simply walk through the list comparing only neighbors (another O(n)). While this does not make much difference of course for smaller lists, the memory requirement indicates much larger lists, and then the difference between O(n^2) and O(n log(n)) may well be the difference between another problem and a solution.


What's wrong with this FizzBuzz? The rules are unsurprisingly:

  Write a program that prints the numbers from 0 to 100. But for multiples of three print “Fizz” instead of the number and for the multiples of five print “Buzz”. For numbers which are multiples of both three and five print “FizzBuzz”."

#include <stdio.h>

int main(void) {
    int i, j;
    i = j = 0;

    for (; i < 100 ; ++i, j = 0, putchar('\n')) {
        j += printf("%s", i % 3 ? "" : "Fizz");
        j += printf("%s", i % 5 ? "" : "Buzz");
        if (!j) printf("%d", i);
    }

    return 0;
}

If you were a job interviewer, would you be ok with an answer like this? Or would you ask for another solution that is not (ab)using the semantics of for and the return value of printf in order to do fewer operations?

The for loop seems fine to me; it isn't abusing any semantics. The semantics of the for loop are quite clear:

for (initialization; loop condition; increment)

Anyone who doesn't know this off the top of their head probably doesn't know the language as well as they think they do. Although you could have written this code using a while loop, it's just a matter of opinion whether that is better. In general, you can transform any for loop into a while loop. When I write a "complex" for loop, I tend to prefer to break each of these components into separate lines for readability (and also to keep the line from getting too long).

However, using the return value of printf the way you did is just wrong. It returns a negative value if the operation failed. You are adding arbitrary positive and negative return values together and checking to see whether the result is non-zero. That is incorrect, and thus decidedly unclever.

You could might say that the test condition could be modified to j > 0, but that's still not precisely correct because one of the printf calls might succeed and return, say, 5, while the other one might fail and return −5. Should the number be printed, or not?

So, while I have no problem with your choice to use a for loop here, I do not like what you've done in the body of that loop. I disagree with cHao's comment that this would be an instant no-hire. I'd probably assume you were a competent programmer and having fun with the inherently-boring FizzBuzz problem. However, I would expect you, with a bit of prompting, to see and begin to explain some of the problems with your chosen approach.

Aside from that…

Keep in mind that code does not become more efficient just because you reduce the number of apparent instructions that you write in C. Optimizing compilers do not perform a literal translation of C instructions into assembly instructions. And even non-optimizing compilers may not be able to do so if they wanted to, because there isn't necessarily a 1-to-1 correspondence of instructions in both languages.

In this case, you're not even gaining any efficiency with the way you've written the printf statements. In fact, the execution of printf is by far the slowest part of the program, so by any real-world performance metrics, avoiding calls to printf whenever possible would be the way to boost performance. Certainly it is not a win to replace a reasonably well-predicted conditional branch with an expensive and pointless function call!

In fact, if you weren't trying to be overly clever, you could substitute puts for printf where you were outputting constant "Fizz" and "Buzz" strings, which would be even more efficient. (Some compilers like GCC will do this translation for you when they can, but they wouldn't be able to do it with the way you'd originally written the code, obviously.)
Stylistically, I would prefer that you kept the variables to as narrow a scope as possible. Since at least C99, you have been allowed to declare variables within a for loop's scope, and this is what I would recommend that you do.
Even more importantly, I prefer to see that variables are initialized at the point of their declaration whenever possible. It bothers me to see lines like:

int i, j;
i = j = 0;

I don't like the attempt to combine initialization of multiple variables onto one line, either. Break them up into two lines. No excuses.


The number of seconds between 2 Dates I have this method in Java:

private static int ConvertToTimestamp(Date value)
{
    DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.S", Locale.US);
    String text = "1970-01-01 00:00:00.0";
    LocalDateTime localDateTime = LocalDateTime.parse(text, formatter);

    // getTime() returns milliseconds
    int seconds = (int) (value.getTime()/1000) - localDateTime.getSecond();
    return seconds;

}

It returns the number of seconds between the input parameter and 1/1/1970.
Are there any improvements to this or better ways to do it?

value.getTime() returns the number of milliseconds since January 1, 1970, 00:00:00 GMT represented by a Date object.

You don't need a ConvertToTimestamp method, just call value.getTime() / 1000 to get the seconds.


Fancy checkbox for web pages I just finished creating a fancy checkbox and am hoping for some tips to help improve my code, as I am still learning.

$(document).ready(function(){
  function checkbox(){
    var y = $("#yon");
    var n = $("#non");
    var cx = $("#chex")
    var cbx = $("#checkbox");
    var chval = false;
    $(".cspan").click(function(){
      $(n).toggleClass("no");
      $(y).toggleClass("yes");
      if(chval){
        $(cx).text("\u2716");
        $(cbx).prop('checked', false);
        chval = false;
      }
      else{
        $(cx).text("\u2714");
  $(cbx).prop('checked', true);
        chval = true;
      }
    });
  }
  checkbox();
});
#checkcon p{

}
input[type="checkbox"] {
    display:none;
}
input[type="checkbox"] + label span {
    display:inline-block;
 width: 160px;
 height: 60px;
 border-radius: 25px;
    margin:-1px 4px 0 0;
    vertical-align:middle;
    background-color: #ff3a31;
    cursor:pointer;
 transition: background-color 100ms linear;
 position:absolute;
}
input[type="checkbox"]:checked + label span {
    background-color: #00cc66;
    transition: background-color 100ms linear;

}
input[type="checkbox"] {

}
.checkemb{
 position:relative;
 display:inline-block;
 margin-right:10px;
 margin-left:10px;
 font-family:"Arial Black", Gadget, sans-serif;
 color:#ffffff;
}
#chex{
 margin-right:25px;
 margin-left:25px;

}
p.yes{
 color:#990800;
 transition: color 100ms linear;
}
p.no{
 color:#008040;
 transition: color 100ms linear;
}

    <input id="checkbx" type="checkbox"/>

Yes

&#10006

No

A few points:

You don't need to declare the checkbox function within the $(document).ready(), you can do it outside it.
You have some empty CSS rules -- by accident?
$(function() {...}); is a shorthand for $(document).ready(function() {...}); -- you might want to use that if you want
You don't need to do $(y), $(n), because you saved y, n as jQuery objects already
When you make the variables y, n, cx, cbx, you can opt to (at the end of the day, it's personal preference, but makes it clear they are 'special' variables) prepend $ (or jQuery) like var $y = $("#yon"), var $n = $("#non"), and so on..., so you know they're jQuery objects.

With that in mind...:

function checkbox() {
  var $y = $("#yon");
  var $n = $("#non");
  var $cx = $("#chex")
  var $cbx = $("#checkbox");
  var chval = false;
  $(".cspan").click(function() {
    $n.toggleClass("no");
    $y.toggleClass("yes");
    if (chval) {
      $cx.text("\u2716");
      $cbx.prop('checked', false);
      chval = false;
    } else {
      $cx.text("\u2714");
      $cbx.prop('checked', true);
      chval = true;
    }
  });
}

$(function() {
  checkbox();
});
input[type="checkbox"] {
  display: none;
}
input[type="checkbox"] + label span {
  display: inline-block;
  width: 160px;
  height: 60px;
  border-radius: 25px;
  margin: -1px 4px 0 0;
  vertical-align: middle;
  background-color: #ff3a31;
  cursor: pointer;
  transition: background-color 100ms linear;
  position: absolute;
}
input[type="checkbox"]:checked + label span {
  background-color: #00cc66;
  transition: background-color 100ms linear;
}
.checkemb {
  position: relative;
  display: inline-block;
  margin-right: 10px;
  margin-left: 10px;
  font-family: "Arial Black", Gadget, sans-serif;
  color: #ffffff;
}
#chex {
  margin-right: 25px;
  margin-left: 25px;
}
p.yes {
  color: #990800;
  transition: color 100ms linear;
}
p.no {
  color: #008040;
  transition: color 100ms linear;
}

    <input id="checkbx" type="checkbox" />

Yes

&#10006

No