Setting value of controller controls for domain objects with Reflection Is my use of Java Reflection an OK design?

Each domain object contains an annotation to declare what controller would be initialized to edit/create the object:

Each field of the domain object has an annotation to declare what control in the specified controller that the user would use to edit its value:

@Edit(controller = EmployeeController.class)
public class Employee extends Item {

     @Node(name = "lastNameField", type = TextField.class)
     private StringProperty lastName = new SimpleStringProperty();

     //Control similar to a TableView
     @Node(name = "contacts", type = ListBox.class)
     private List <Contact> contacts;

}

public @interface Edit {
     Class<? extends ModifyEntityController> controller();
}

public @interface Node {
     String name();
     Class<?> type();
 }

 public Class EmployeeController {
         @FXML
         private TextField lastNameField;
    }

Double-clicking on a ListCell would call .edit() on the domain object.

This method is in the parent class Item.

.edit() does: example = employee.edit();

create an instance of the @Edit controller
use @Node  to create a binding between the specified control
set the root node of controller to the main window of the application.

//create an instance of the controller
Edit edit = getClass().getAnnotation(Edit.class);
Class<? extends Controller> controller = edit.controller();

Object controllerInstance = null;

try {
    controllerInstance = edit.controller().newInstance();
} catch (Exception e) {
    e.printStackTrace();
}

//create bindings between the specified controls
List <Field> itemFields = FieldUtils.getFieldsListWithAnnotation(getClass(), Node.class);

for (Field field : itemFields ) {
     Node node = field.getAnnotation(Node.class);
     Object itemObject = FieldUtils.readField(field, this, true);
     Object controllerControl = FieldUtils.readDeclaredField(controllerInstance, node.name(), true);

     if (node.type() == TextField.class) {
          TextInputControl tc = (TextInputControl) controllerControl;
          StringProperty sp = (StringProperty) itemObject;
          tc.textProperty().bindBidirectional(sp);
    }

     else if (node.type() == ListBox.class) {
          ListBox lb =  (ListBox) controllerControl;
          List <CellObject> list = (List<CellObject>) itemObject;
          lb.setList(FXCollections.observableArrayList(list));
    }

}

*The implementation of a ListBox is actually not a binding of properties but setting the value of its ListView to the List Field of the domain object.
All objects that would be the value of a ListCell would implement CellObject.

//set the root node of controller to the main window of the application
AgendaApp.setContent(Controller) controllerInstance);

I went with reflection as the controllers to update/create domain objects consist of mainly TextFields and ListBox's.  This would save a lot of time writing the application.

Is my use of Java Reflection an OK design?

Well, no, the usage of reflection is usually an indication of bad design. Honestly, it is almost always. Actually, you have to change the question and ask, when is it okay, to use reflection. The answer is something similar like this: Use reflection, when you do not know how a class looks like during runtime. But this is your code, so, why would you increase complexity and make your code more error prone?

In general, I have some worries, when I read the code:

@Node(name = "lastNameField", type = TextField.class)
private StringProperty lastName = new SimpleStringProperty();

Now, it might be a language or a definition subject, but if Employee is a domain model, it must not have dependencies to the view layer, at least no direct dependencies.

@Edit(controller = EmployeeController.class)
public class Employee extends Item {

The Employee itself is in control, of who controls itself. Shouldn't you tell the EmployeController, which model it shall edit?

Double-clicking on a ListCell would call .edit() on the domain object.

Same thing. The presentation code calls the domain object, then, the domain objects calls someone and says "someone wants to edit me". If I understood it correctly.

There's some other fishy stuff, but I'll conclude now, because in my opinion there are some problems even without the reflection stuff: It seems like an over engineered approach for an easy problem. Or: It looks like the solution of a problem, which probably isn't even a problem. It certainly is, from my point of view, the wrong tool for "the problem". The usage of reflection makes maintaining the code harder and more error prone, at least in my experience. The usage of annotations, too, by the way.

I'd suggest to take a step back to the solution before you implemented reflection and analyze what object oriented principles are violated (e.g. SOLID, coupling, cohesion,...) and what patterns can help you get rid of those problems. Now, that's the major point: Ask what design patterns help you get rid of your design problems, do not ask would reflection get rid of the design problems?.

I recommend fowler's web page about gui architectures: https://martinfowler.com/eaaDev/ (presentation patterns). It's not exactly a "good read", but it certainly is educational. It helps you to understand, which presentation patterns suits your application best and gives you some guidelines about how to implement you presentation layer.


Four Elements from GeeksForGeeks This is a question from GeeksForGeeks

Given an array of integers, find a combination of four elements in the
array whose sum is equal to a given value X.
Input: First line consists of T test cases. First line of every test
case consists of an integer N, denoting the number of elements in an
array. Second line consists of N spaced array elements. Third line of
every test case X.
Output: Single line output, print 1 if combination is found else 0.
Constraints: 1<=T<=100
1<=N<=100
Example: Input: 1 6 1 5 1 0 6 0 7
Output: 1

Here is the code
#include <iostream>
#include <cassert>
#include <vector>

int main() {
    int t, n, val, x, sum = 0;
    bool found = false;
    std::cin >> t;
    assert(t >= 1 && t <= 100);

    while (t--)
    {
        std::cin >> n;
        assert(n >=1 && n <= 100);
        std::vector<int> vec(n);
        for (int i = 0; i < n; i++)
        {
            std::cin >> val;
            vec[i] = val;
        }

        std::cin >> x;

        for (int i = 0; i < n - 3; i++)
        {
            for (int j = i + 1; j < n - 2; j++)
            {
                for (int k = j + 1; k < n - 1; k++)
                {
                    for (int l = k + 1; l < n; l++)
                    {
                        sum = vec[i] + vec[j] + vec[k] + vec[l];
                        if (sum == x)
                        {
                            found = true;
                            break;
                        }
                    }
                }
            }
        }

        if (found == true)
        {
            std::cout << 1 << std::endl;
        }
        else
        {
            std::cout << 0 << std::endl;
        }
    }
    return 0;
}

How to optimise this code?

When to use Assert
Generally assert() is a debugging tool. The GeekforGeeks website itself warns against using it in certain cases.

If the NODEBUG compilation flag is used in the file the assertion is removed.

It might be better to use if statements and write the output to std::cerr. That would provide a much more meaningful error message.

Reduce Complexity, Follow SRP
The Single Responsibility Principle states that every module or class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility.

  Robert C. Martin expresses the principle as follows:
          A class should have only one reason to change.

While this is primarily targeted at classes in object oriented languages it applies to functions and subroutines well.

The more separate functions there are the easier it is to understand or read the code. This also makes it easier for any programmer to maintain or debug the code.

The main() function could be broken up into at multiple functions. The main() function should generally just control the program and handle any exceptions that get to that point.

An example of a decent main() function for this program might be:

int main() {
    int t;
    std::cin >> t;
    assert(t >= 1 && t <= 100);
    int x;

    while (t--)
    {
        std::vector<int> vec = getTestCase(x);

        std::cout << ((executeTestCase(vec, x)) ? 1 : 0) << std::endl;
    }

    return 0;
}

The first function that might be of use is an test case input function.

std::vector<int> getTestCase(int &x)
{
    int n;
    std::cin >> n;
    assert(n >= 1 && n <= 100);
    std::vector<int> vec(n);
    for (int i = 0; i < n; i++)
    {
        int val;
        std::cin >> val;
        vec.push_back(val);
    }

    std::cin >> x;

    return vec;
}

The second function that might be of use would be a that finds if there are values that meet the proper conditions.

bool executeTestCase(std::vector<int> vec, int x)
{
    bool found = false;
    int sum = 0;
    int n = vec.size();

    for (int i = 0; i < n - 3; i++)
    {
        for (int j = i + 1; j < n - 2; j++)
        {
            for (int k = j + 1; k < n - 1; k++)
            {
                for (int l = k + 1; l < n; l++)
                {
                    sum = vec[i] + vec[j] + vec[k] + vec[l];
                    if (sum == x)
                    {
                        found = true;
                        break;
                    }
                }
            }
        }
    }

    return found;
}

Improve Performance
Since the code is using a C++ container class, it might be possible to use iterators to improve performance. Once the code is broken up into functions and compiled -O3 it can be profiled to find where any bottlenecks exist.


Simple AJAX request application Today I created my first simple AJAX application. It works fine, but I am a bit unsure if I did everything correct. I'd like to use the code as template for future projects.

function _solve(id) {
    var xhr = new XMLHttpRequest();
    xhr.onreadystatechange = function () {
        var DONE = 4; // readyState 4 means the request is done.
        var OK = 200; // status 200 is a successful return.
        if (xhr.readyState === DONE) {
            if (xhr.status === OK) {
                obj = JSON.parse(xhr.responseText);
                if (obj.success) {
                    if (document.getElementById('line'+obj.id).style.display != 'none') {
                        document.getElementById('line'+obj.id).style.display = 'none';
                        document.getElementById('count').innerHTML = document.getElementById('count').innerHTML - 1;
                    }
                } else {
                    alert('Json error: ' + obj.error);
                }
            } else {
                alert('Error: ' + xhr.status); // An error occurred during the request.
            }
        }
    };
    xhr.open('POST', 'ajax_cmd.php', true);
    xhr.setRequestHeader('Content-type', 'application/x-www-form-urlencoded');
    xhr.send('cmd=solve&id='+id);
}

Beside general improvement suggestions, I have following particular questions:

Do I need to somehow close the connection? It looks like send() automatically closes the connection because I cannot execute send() twice.
Is the error handling ok? When my internet connection was a bit unstable, I got the message "Error: 0". Zero sounds like a strange status code.

Note: For now, I'd like to stick with plain JavaScript, without jQuery or other frameworks.

Explanation/context of the code:

The HTML page has a table. Each table row  represents a dataset with an ID. The id of each row is "line" plus the dataset ID, e.g. "line1". Each table row has a link named "Solve" which calls the JavaScript function _solve(id).

  php/error.log
  Solve
  1113
  2017-09-03&nbsp;01:18:18
  Xyz...

When the user clicks at "Solve", a PHP script (ajax_cmd.php) is called with AJAX. The PHP script deletes the dataset and confirms the deletion with the JSON object success: true; id: (the ID) on success or success: false; error: (the error message) on failure.

After the PHP script confirmed the successful deletion, the AJAX script hides the table row and decreases a row counter.

A double-click prevention (which might result in a double-decrease of the counter) is realized with a check if the row is already hidden.

Let's start off with your questions:

  Do I need to somehow close the connection? It looks like send() automatically closes the connection because I cannot execute send() twice.

No. The browser abstracts all connection details from you, like connection persistence, caching, protocols, etc. You just have to think that an instance of XMLHttpRequest represents one request-response pair.

  Is the error handling ok? When my internet connection was a bit unstable, I got the message "Error: 0". Zero sounds like a strange status code.

A zero status code is a non-HTTP status code for a generic network failure. You can get that status code from invalid certificate errors, cross-domain errors, extensions blocking the connection, etc. On JS, you often cannot get any specific information from the error, and the only thing you can do is show a generic error message on your app. However, the browser console will tell you what happened exactly.

var xhr = new XMLHttpRequest();

Is so 2010. There's a simpler fetch that does more with a simpler syntax. Consider using that instead. It's inspired greatly by jQuery. If you're familiar with jQuery, you pretty much know what to do.

    var DONE = 4; // readyState 4 means the request is done.
    var OK = 200; // status 200 is a successful return.
    if (xhr.readyState === DONE) {
        if (xhr.status === OK) {

Unless you're reusing DONE and OK elsewhere, it's pointless to put them in variables. If one sees xhr code done manually, it's pretty much standard to check readyState 4 and status 200 when expecting a success.

obj = JSON.parse(xhr.responseText);

JSON.parse can throw an error when the string is malformed JSON. And malformed JSON can be caused by bad server code, or a misconfigured server that returns HTML error pages on errors. Wrap this in a try-catch, and fail the operation if it does throw.


A more generic `DelegateCommand` In an effort to create ever-improving code, I've written a DelegateCommand that takes a generic type parameter to be acted upon. It's pretty trivial, so it should hopefully be easy to review.

In order to do this, I had to create two interfaces: IGenericCommand and IWindowsUniversalCommand.

///
/// Requires the implementation of System.Windows.Input.ICommand as well.
///
/// <typeparam name="T"></typeparam>
public interface IGenericCommand<T> : ICommand
{
    ///
    /// A generic version of System.Windows.Input.ICommand.CanExecute(object).
    ///
    /// <param name="parameter"></param>
    /// <returns></returns>
    bool CanExecute(T parameter);
    ///
    /// A generic version of System.Windows.Input.ICommand.Execute(object).
    ///
    /// <param name="parameter"></param>
    void Execute(T parameter);
}

///
/// Requires the implementation of System.Windows.Input.ICommand as well.
///
interface IWindowsUniversalCommand : ICommand
{
    ///
    /// Should refresh whether or not the ICommand can execute, or call System.Windows.Input.ICommand.CanExecuteChanged.
    ///
    void RaiseCanExecuteChanged();
}

Then I created an abstract class WindowsUniversalCommand which implements some of the basic logic needed for the command, to save from having to repeat it regularly. (Just in case I create other commands for Windows Universal Apps.)

public abstract class WindowsUniversalCommand : IWindowsUniversalCommand
{
    public abstract bool CanExecute(object parameter);
    public abstract void Execute(object parameter);

#if WINDOWS_UWP
    // The .NET version on Windows RT (Universal Apps) does not support the `CommandManager` trick below, so we just make a regular event and fire it.
    public event EventHandler CanExecuteChanged;
#else
    // Programmes that are not Windows Universal Apps support the `CommandManager` trick.
    public event EventHandler CanExecuteChanged
    {
        add { CommandManager.RequerySuggested += value; }
        remove { CommandManager.RequerySuggested -= value; }
    }
#endif

    protected virtual void OnCanExecuteChanged(EventArgs e)
    {
        var handler = CanExecuteChanged;
        handler?.Invoke(this, e);
    }

    ///
    /// This method is required for Windows Universal Apps (WinRT). Non-WinRT programmes may simply ignore this method.
    ///
    public void RaiseCanExecuteChanged() => OnCanExecuteChanged(EventArgs.Empty);
}

The two abstract methods are required so that WindowsUniversalCommand can satisfy the ICommand interface.

Lastly, I have a DelegateCommand<T>.

public class DelegateCommand<T> : WindowsUniversalCommand, IGenericCommand<T>
{
    private readonly Func<T, bool> _canExecuteMethod;
    private readonly Action<T> _executeMethod;

    public DelegateCommand(Action<T> executeMethod, Func<T, bool> canExecuteMethod = null)
    {
        _executeMethod = executeMethod;
        _canExecuteMethod = canExecuteMethod;
    }

    public override bool CanExecute(object parameter) => parameter is T ? CanExecute((T)parameter) : false;

    public override void Execute(object parameter) => Execute((T)parameter);

    public bool CanExecute(T parameter) => _canExecuteMethod == null || _canExecuteMethod.Invoke(parameter);

    public void Execute(T parameter) => _executeMethod?.Invoke(parameter);
}

public override bool CanExecute(object parameter) => parameter is T ? CanExecute((T)parameter) : false;

Beware of a lesser known gotcha. This may not work as expected for parameter that's not "really" a T (it's not in Ts inheritance tree), only has an explicit cast operator defined allowing it to be casted to T.

In that case while (T) parameter would return a T instance,  CanExecute won't even be called because your is check would prevent this from happening.

Other than that, I'd have some remarks related to naming and code style (rather subjective, so treat them as suggestions or food for thought more than claims your code is incorrect)

public interface IGenericCommand<T> : ICommand

There's no use in putting "generic" in the name. Any C# programmer knows that <T> indicates generics by itself. Case in point: in .NET we've got IEnumerable and IEnumerable<T> - not IGenericEnumerable<T>. (So "drop the the" ;) )

/// <typeparam name="T"></typeparam>

/// <param name="parameter"></param>
/// <returns></returns>

In my opinion such empty tags are of no use, all they do is clutter the codebase.

private readonly Func<T, bool> _canExecuteMethod;
private readonly Action<T> _executeMethod;

It's a matter of taste, but here I would ditch the "Method" suffix, as these aren't necessarily "methods". Note that you didn't name the class MethodCommand. Why not just _canExecute and _onExecute?

public bool CanExecute(T parameter) => _canExecuteMethod == null || _canExecuteMethod.Invoke(parameter);

Since you're using the null safety operator already, this could be simplified to _canExecuteMethod?.Invoke(parameter) ?? true.


Image processing optimization I'm using Winforms C# .NET 3.5. I'm getting frames, and this is how I handle them:

    delegate void videoStream_NewFrameDelegate(object sender, NewFrameEventArgs eventArgs);
    public void videoStream_NewFrame(object sender, NewFrameEventArgs eventArgs)
    {
        if (ready)
        {
            if (this.InvokeRequired)
            {
                videoStream_NewFrameDelegate del = new videoStream_NewFrameDelegate(videoStream_NewFrame);
                this.Invoke(del, new object[] {sender, eventArgs} );
            }
            else
            {
                Rectangle rc = ClientRectangle;
                Bitmap bmp = new Bitmap(rc.Width, rc.Height);
                Graphics g = Graphics.FromImage((Image)bmp);
                g.InterpolationMode = System.Drawing.Drawing2D.InterpolationMode.HighQualityBilinear;
                g.DrawImage((Bitmap)eventArgs.Frame, rc.X+10, rc.Y+10, rc.Width-20, rc.Height-20);

                g.Dispose();
                this.Image = (Image)bmp;
            }
        }
    }

Is there any way to optimize or improve performance?

I'm getting low performance when my image is 320x240 and I'm stretching it to 1280x1024, But the same image on 640x480 and stretching to 1280x1024 wont get much loss in performance.

I tried to use WPF and still same performance loss. That is weird because WPF is supposed to use DirectX and be fast in image processing.

Here is my WPF code:

    delegate void videoStream_NewFrameDelegate(object sender, NewFrameEventArgs eventArgs);
    public void videoStream_NewFrame(object sender, NewFrameEventArgs eventArgs)
    {
        if (ready)
        {
            if (!this.Frame.Dispatcher.CheckAccess())
            {
                videoStream_NewFrameDelegate del = new videoStream_NewFrameDelegate(videoStream_NewFrame);
                this.Frame.Dispatcher.Invoke(del, new object[] { sender, eventArgs });
            }
            else
            {
                Bitmap bmp = (Bitmap)eventArgs.Frame.Clone();
                IntPtr hBitmap = bmp.GetHbitmap();
                BitmapSource img = Imaging.CreateBitmapSourceFromHBitmap(hBitmap, IntPtr.Zero, Int32Rect.Empty, BitmapSizeOptions.FromEmptyOptions());
                bmp.Dispose();
                GC.Collect();

                this.Frame.Source = img;
            }
        }
    }

I tried to use WPF and still same preformence lose.. that is weird because WPF suppose to use DirectX and be fast in image proccessing.

Except, in the WPF version, you are cloning a Bitmap (which has nothing to do with WPF) and then calling Imaging.CreateBitmapFromHBitmap, which eventually resolves to a call to:

[DllImport("WindowsCodecs.dll", EntryPoint="IWICImagingFactory_CreateBitmapFromHBITMAP_Proxy")]
internal static extern int CreateBitmapFromHBITMAP(IntPtr THIS_PTR, IntPtr hBitmap, IntPtr hPalette, WICBitmapAlphaChannelOption options, out BitmapSourceSafeMILHandle ppIBitmap);

Now I don't know what that method is doing, but it may very well be copying the data again, but perhaps not.  Either way, your WPF method is performing at least one clone of the entire image and your WinForms version is not.

That's not really the meat of it though.  You haven't even posted benchmark results, so you need to do that first before assuming any one part of code is "slow", and you will have a hard time optimizing until you have that answer.  You're two versions aren't even identical in output; the WinForms version performs HighQualityBilinear interpolation, which is certainly going to take a significant amount of time.

You need to tell us what you requirements are.  I ran a quick test with the following code using a 320x240 size image, and then again at 640x480:

public Form1()
{
    InitializeComponent();
}

private TimeSpan RunTest( int sampleSize )
{
    var sw = Stopwatch.StartNew();
    var baseImg = new Bitmap( @"C:\TestImage.jpg" );
    Bitmap bmp = null;
    for( int i = 0; i < sampleSize; ++i )
    {
        var rect = picBox.ClientRectangle;
        bmp = new Bitmap( rect.Width, rect.Height );
        using( Graphics g = Graphics.FromImage( bmp ) )
        {
            g.InterpolationMode = InterpolationMode.HighQualityBilinear;
            g.DrawImage( baseImg, rect );
        }

        picBox.Image = bmp;
            // the call to Invalidate() that you have is not needed.
    }

    sw.Stop();
    return TimeSpan.FromMilliseconds( sw.ElapsedMilliseconds );
}

private void picBox_Click( object sender, EventArgs e )
{
    int iterations = 100;
    var time = RunTest( iterations );
    MessageBox.Show( String.Format(
        "Elapsed: {0}, Average: {1}",
        time.TotalMilliseconds,
        time.TotalMilliseconds / iterations ) );
}

I then performed another test using the default InterpolationMode.  Under a release build that gave me the following results:

320x240
1. High quality interpolation: Total: 3122ms, Average: 31.22ms, FPS: 32
2. Default interpolation: Total: 2165ms, Average: 21.65ms, FPS: 46

640x480
1. High quality interpolation: Total: 3963ms, Average: 39.63ms, FPS: 25
2. Default interpolation: Total: 2256ms, Average: 22.56ms, FPS: 44

Are you saying that 32 frames per second is not good enough for your application?  Is 42 ok and is it alright to remove the high quality rendering?  You need to provide more details.  You can't expect us to optimize your code with no target requirement in mind.  Also, as you can see, your claim that the performance is better with large images does not appear to be true, though the difference without high quality bilinear rendering is negligible.

If you can forego the Bilinear interpolation you can simply set the StretchMode of the PictureBox to Stretch and just set the Image property to the Frame property of the event args object and be done with it.  This will be significantly faster.


Method for obtaining adjacent chunk positions I'm currently in the process of making a 2D tile based game. This game has an "infinite" world that can be traversed and modified as needed. I am creating the chunks with a mesh and I apply a texture to them. The chunks texture updates every time I set a tile with a new Sprite.

Each time I set a new tile, the surrounding tiles from the same chunk recalculate based on their adjacent tiles and they set their sprite accordingly (Auto-tiling). This works perfectly fine and I have no issues, unless the tile is along the edge of the chunk.

If the tile is along the edge of the chunk, it updates the texture of the chunk perfectly but the adjacent chunk textures aren't updated. Now, I am using the below code to obtain the adjacent chunks based on the tiles position. This works, however it is extremely ugly and unreadable.

if (tileX == 0 || tileY == 0 || tileX == chunkSize - 1 || tileY == chunkSize - 1)
{
    var chunksToUpdate = new List<Vector3Int> ();
    var position = chunk.Position;

    if (tileX == 0 && tileY == 0)
    {
        chunksToUpdate.Add (new Vector3Int (position.X - chunkSize, position.Y, position.Z)); //left
        chunksToUpdate.Add (new Vector3Int (position.X, position.Y - chunkSize, position.Z)); //down
        chunksToUpdate.Add (new Vector3Int (position.X - chunkSize, position.Y - chunkSize, position.Z)); //downleft

    }
    else if (tileX == 0 && tileY == chunkSize - 1)
    {
        chunksToUpdate.Add (new Vector3Int (position.X - chunkSize, position.Y, position.Z)); //left
        chunksToUpdate.Add (new Vector3Int (position.X, position.Y + chunkSize, position.Z)); //up
        chunksToUpdate.Add (new Vector3Int (position.X - chunkSize, position.Y + chunkSize, position.Z)); //upleft

    }
    else if (tileX == chunkSize - 1 && tileY == 0)
    {
        chunksToUpdate.Add (new Vector3Int (position.X + chunkSize, position.Y, position.Z)); //right
        chunksToUpdate.Add (new Vector3Int (position.X, position.Y - chunkSize, position.Z)); //down
        chunksToUpdate.Add (new Vector3Int (position.X + chunkSize, position.Y - chunkSize, position.Z)); //downright

    }
    else if (tileX == chunkSize - 1 && tileY == chunkSize - 1)
    {
        chunksToUpdate.Add (new Vector3Int (position.X, position.Y + chunkSize, position.Z)); //up
        chunksToUpdate.Add (new Vector3Int (position.X + chunkSize, position.Y, position.Z)); //right
        chunksToUpdate.Add (new Vector3Int (position.X + chunkSize, position.Y + chunkSize, position.Z)); //upright

    }
    else if (tileX == 0)
    {
        chunksToUpdate.Add (new Vector3Int (position.X, position.Y + chunkSize, position.Z)); //up

    }
    else if (tileX == chunkSize - 1)
    {
        chunksToUpdate.Add (new Vector3Int (position.X + chunkSize, position.Y, position.Z)); //right

    }
    else if (tileY == 0)
    {
        chunksToUpdate.Add (new Vector3Int (position.X, position.Y - chunkSize, position.Z)); //down

    }
    else if (tileY == chunkSize - 1)
    {
        chunksToUpdate.Add (new Vector3Int (position.X, position.Y + chunkSize, position.Z)); //up

    }

Clarification: I am checking to see if tileX or tileY are along the edges of the chunk. If they are then I add a new Vector3Int to the list. I also only attempt to obtain the chunks that will be affected by the chunks texture update.

After this, I loop through all of the Vector3Int's in the chunksToUpdate list and update them like so.

foreach (var pos in chunksToUpdate)
{
    MeshChunk updateChunk = GetChunk (pos);

    if (updateChunk != null)
    {
        UpdateChunkTiles (updateChunk);
    }
}

I'm not looking for any criticism about my naming conventions or formatting. I'm wondering if there is a more elegant way of handling this scenario? Can it be refactored to take up less space and its readability improved.

Well, I suggest next approach

        if (tileX != 0 && tileY != 0 && tileX != chunkSize - 1 && tileY != chunkSize - 1)
        {
            return;
        }

        var leftChunk = tileX == 0;
        var rightChunk = tileX == chunkSize - 1;
        var downChunk = tileY == 0;
        var upChunk = tileY == chunkSize - 1;

        if (leftChunk)
        {
            chunksToUpdate.Add(new Vector3Int(position.X - chunkSize, position.Y, position.Z)); // Left
            if (downChunk)
            {
                chunksToUpdate.Add(new Vector3Int(position.X - chunkSize, position.Y - chunkSize, position.Z)); // DownLeft
            }
            else if (upChunk)
            {
                chunksToUpdate.Add(new Vector3Int(position.X - chunkSize, position.Y + chunkSize, position.Z)); // UpLeft
            }
        }
        else if (rightChunk)
        {
            chunksToUpdate.Add(new Vector3Int(position.X + chunkSize, position.Y, position.Z)); // Right
            if (upChunk)
            {
                chunksToUpdate.Add(new Vector3Int(position.X + chunkSize, position.Y + chunkSize, position.Z)); // UpRight
            }
            else if (downChunk)
            {
                chunksToUpdate.Add(new Vector3Int(position.X + chunkSize, position.Y - chunkSize, position.Z)); // DownRight
            }
        }
        if (downChunk)
        {
            chunksToUpdate.Add(new Vector3Int(position.X, position.Y - chunkSize, position.Z)); // Down
        }
        else if (upChunk)
        {
            chunksToUpdate.Add(new Vector3Int(position.X, position.Y + chunkSize, position.Z)); // Up
        }

It makes code a little bit more readable by using bool named variables and also has less code lines and if else branching.

Moreover, it is often (almost every time) better to have less nesting via inverting if statement like I did. So basically I quit if it is not an edge tile.


Is this system to block bots reliable? I've got a page which contains a form: when page is requested, a session is stored:

$_SESSION['ok']=date("His");

Plus, the form contains this hidden field:

<input type="text" name="email-confirm" id="email-confirm" style="display:none;" placeholder="Do not fill this field">

When user sends the form, the following PHP code is launched to check if they're not a bot:

session_start();
if(!isset($_SESSION['ok'])){die('Error: you have to enable cookies in order to make everything work fine.');}
$fast = date("His") -4;
if($fast < $_SESSION['ok']){$_SESSION['banned']=1;
die('Woah, that was fast! Would you mind doing a little captcha verification? Refresh the page to continue.');}
if(!empty($_POST['email-confirm'])){$_SESSION['banned']=1;
die('You\'ve been temporarily banned.<br>
Refresh the page to lift the ban with a captcha verification.');
}

If session 'banned' is set and user tries to visit any page on the site, they're asked to complete a captcha challenge which relies on google recaptcha.

Is this method to block bots reliable?
I don't like very much the idea to ask every user to complete a captcha verification, because  I feel like it would be annoying to do that every time, but if it's the only method to be really safe, then I'd adopt it.

Firstly your formatting looks god awful - I hope that this is just from pasting it here.

Then you should rename your session variable. It isn't ok it's the time of last request - so $_SESSION['lastRequest'] seems like a good name.
Also that 4 seconds shouldn't be just some random number but a named constant that is used in all places and then can be changed easily.

Now about the effectiveness:
I don't know what the latest advancements in the bot-scene are. However I'm quite certain that XRumer still is one of the better ones, so I'll try to match your system against that.

Your strongest protection is the captcha. There always are claims that those are bypassed - you can half the boasted recognition rate and probably still be on the safe side.

Your email-confirm trick is easily detectable - just check for the display:none and not fill it out.
Whether that is actually done depends on some factors:

Whether the bot is even able to do this. But detecting the trick in the current form isn't hard - I'm quite certain XRumer is able to detect that.
Whether the bot WANTS to do that. Spamming is only about volume. Parsing the site to detect such cheap tricks takes time, memory and CPU load.
Most probably the spammers do not care about your site in particular; They might set their spambot to the fastest mode and blast away. Maybe at different times the bot is configured to try the best it can to post its message - and your current trick fools no one.

However you can just create another CSS id for the hidden fields and use that.
As said, spamming is mostly done as quick as possible. Downloading and parsing further files is avoided - and I'm certain it will stay that way.

So in conclusion: It may not stop everything, but it will stop a lot.


Make it easier with color changing How can I simplify this in Delphi?

Procedure colori1
Begin

 if Temperatura<=15 then
 begin
  Form1.Label1.Font.Color:=clBlue;
  Form1.Label2.Font.Color:=clBlue;
  Form1.Label3.Font.Color:=clBlue;
  Form1.Label4.Font.Color:=clBlue;
  Form1.Label5.Font.Color:=clBlue;
 end;

 if (Temperatura>=16) and (Temperatura<=18) then
 begin
  Form1.Label1.Font.Color:=clAqua;
  Form1.Label2.Font.Color:=clAqua;
  Form1.Label3.Font.Color:=clAqua;
  Form1.Label4.Font.Color:=clAqua;
  Form1.Label5.Font.Color:=clAqua;
 end;

 if (Temperatura>=19) and (Temperatura<=22) then
 begin
  Form1.Label1.Font.Color:=clLime;
  Form1.Label2.Font.Color:=clLime;
  Form1.Label3.Font.Color:=clLime;
  Form1.Label4.Font.Color:=clLime;
  Form1.Label5.Font.Color:=clLime;
 end;

 if (Temperatura>=23) and (Temperatura<=26) then
 begin
  Form1.Label1.Font.Color:=clYellow;
  Form1.Label2.Font.Color:=clYellow;
  Form1.Label3.Font.Color:=clYellow;
  Form1.Label4.Font.Color:=clYellow;
  Form1.Label5.Font.Color:=clYellow;
 end;

 if (Temperatura>=27) and (Temperatura<=29) then
 begin
  Form1.Label1.Font.Color:=$000080FF;
  Form1.Label2.Font.Color:=$000080FF;
  Form1.Label3.Font.Color:=$000080FF;
  Form1.Label4.Font.Color:=$000080FF;
  Form1.Label5.Font.Color:=$000080FF;
 end;

 if Temperatura>=30 then
 begin
  Form1.Label1.Font.Color:=clRed;
  Form1.Label2.Font.Color:=clRed;
  Form1.Label3.Font.Color:=clRed;
  Form1.Label4.Font.Color:=clRed;
  Form1.Label5.Font.Color:=clRed;
 end;
end;

If I use "for cycle" like this:

For i:=0 to n do label[i].font.color:=clRed

I'll obviously get an error, because Delphi doesn't know what label[i] means. Any suggestions?

Declare a local variable to hold the color:

var
  Color: TColor;

Then decide what the color should be:

if Temperatura<=15 then
  Color:=clBlue;
else if Temperatura<=18 then
  Color:=clAqua;
else ...

Then assign the color to the controls:

Form1.Label1.Font.Color:=Color;
Form1.Label2.Font.Color:=Color;
....

The labels could be stored in an array or a list. So that you can iterate over them to assign the color. You could declare the array like this, in the form class:

FLabels: TArray<TLabel>;

In the constructor assign it like this:

FLabels := TArray<TLabel>.Create(Label1, Label2, Label3, Label4, Label5);

To iterate over it setting the color do this:

var
  lbl: Tlabel;
....
for lbl in FLabels do
  lbl.Font.Color:=Color;

You appear to be using a global variable Form1. Your code will be better without that global variable, and having this procedure (and others like it) converted into a method of the form.


Inspector interface serializer I've written some code that allows Unity3D's inspector to display fields that conform to an interface. Unity has some quirks about their inspector, so as a preface they are listed here:

If you add a [Serializable] attribute to a class, Unity's Inspector will attempt to show all public fields inside that class.
Any class extending Monobehaviour automatically has the [Serializable] attribute
Unity's inspector will attempt to display any private field with the [SerializeField] attribute.
Unity's inspector will not attempt to display generic types or interfaces, with the exception of List<T>, which is hard-coded.
Unity's inspector will not attempt to display properties. A common workaround is to have a private backing field for your property with [SerializeField] attached. Setters won't be called on the value set in the inspector. It's typically only set pre-compilation time, although a developer can modify values in the inspector during runtime. Currently it is acceptable to me to only take the initial value, although if anybody has a simple and efficient way to update successfully, I'd be happy to hear it.
Unity has a PropertyDrawer class you can extend to control how a type is displayed in the inspector. The PropertyDrawer for an interface or generic type will be ignored.

The code

UnityInterfaceHelper.cs

[Serializable]
public class UnityInterfaceHelperBase
{
    [Tooltip("The component that is of the type required.")]
    [SerializeField]
    public Component target;
}

[Serializable]
public class UnityInterfaceHelper<TInterface> where TInterface : class
{
    public TInterface TargetAsInterface
    {
        get
        {
            if (targetAsInterface == null)
            {
                targetAsInterface = target as TInterface;
            }
            return targetAsInterface;
        }

        set
        {
            if (targetAsInterface != value)
            {
                targetAsInterface = value;
                if (value as Component != null)
                {
                    target = targetAsInterface as Component;
                }
            }
        }
    }

    [Tooltip("The component that is of the type required.")]
    [SerializeField]
    private Component target;

    private TInterface targetAsInterface;

    public static implicit operator UnityInterfaceHelper<TInterface>(UnityInterfaceHelperBase b)
    {
        return new UnityInterfaceHelper<TInterface>()
        {
            target = b.target
        };
    }

    public static implicit operator UnityInterfaceHelperBase(UnityInterfaceHelper<TInterface> b)
    {
        return new UnityInterfaceHelperBase()
            {
                target = b.target
            };
    }
}

UnityInterfaceHelperPropertyDrawer.cs

[CustomPropertyDrawer(typeof(UnityInterfaceHelperBase), true)]
public class UnityInterfaceHelperPropertyDrawer : PropertyDrawer
{
    public override void OnGUI(Rect position, SerializedProperty property, GUIContent label)
    {
        label = EditorGUI.BeginProperty(position, label, property);

            position = EditorGUI.PrefixLabel(position, GUIUtility.GetControlID(FocusType.Passive), label);
            EditorGUI.PropertyField(position, property.FindPropertyRelative("target"), GUIContent.none);

        EditorGUI.EndProperty();
    }
}

Typical usage

[SerializeField]
private UnityInterfaceHelperBase itemComparable;

public IComparable ItemComparable
{
    get { return ((UnityInterfaceHelper<IComparable>)itemComparable).TargetAsInterface; }
    set { ((UnityInterfaceHelper<IComparable>)landedComparable).TargetAsInterface = value; }
}

public void CompareItems()
{
    if(ItemComparable.CompareTo("Hello") == 0)
    {
        Debug.Log("Hello world!");
    }
}

I'm looking to reduce the amount of code I have to repeat for every property, it's already down quite a way, but I'm hoping to make it as simple as possible, but any other comments on the code are welcome too.

Possible bug:

If you get the value, targetAsInterface is initialized. One can then set the value to something that is not a Component. Like null.

If you then get the value again, you'll get the old value of target recasted to TInterface. Seems to me that this violates what get and set are supposed to do.


Detecting a cycle in a linked list using Java I'm solving HackerRank "Linked Lists: Detect a Cycle" challenge.

  A linked list is said to contain a cycle if any node is visited more
  than once while traversing the list.

  Complete the function provided in the editor below. It has one
  parameter: a pointer to a Node object named head that points to the head
  of a linked list. Your function must return a boolean denoting whether
  or not there is a cycle in the list. If there is a cycle, return true;
  otherwise, return false.

  Note: If the list is empty, head will be null.

/*
Detect a cycle in a linked list. Note that the head pointer may be 'null' if the list is empty.

A Node is defined as:
    class Node {
        int data;
        Node next;
    }
*/

boolean hasCycle(Node head) {
    HashSet<Node> seen = new HashSet<Node>();

    Node current = head;
    while (current != null && current.next != null) {
        if (seen.contains(current)) {
            return true;
        }

        seen.add(current);
        current = current.next;
    }

    return false;
}

As a beginner Java developer, what can be improved here?

The method should be public.

Testing for the current.next != null condition is unnecessary. Also, consider writing the loop as a for loop for clarity:

for (Node current = head; current != null; current = current.next) {
    …
}

You don't need to call seen.contains(), because seen.add() will return false if the item you are adding is already in the set.


Consolidate .on function I have two .on() functions for separate dynamically added elements, they are:

$('body').on('click', 'img', function() {
   modal(content[$('li.active a').attr("href")].images[$("img").index(this)]);
});
$('body').on('click', 'img', function() {
   closeM();
});

The second function, I have shortened to

$("body").on("click", "#close", closeM);

In looking at the jQuery documentation for .on(), I saw that I can do something like:

$("body").on({
  click: function(){
  },
  mouseenter: function() {
  }
});

I was wondering if I could either do something like that with my previous two functions, or if there was any other way of shortening/combining them.

You could just do both operations inside the anonymous function (that you are sending as third parameter).

$('body').on('click', 'img', function() {
   modal(content[$('li.active a').attr("href")].images[$("img").index(this)]);
   closeM();
});

Also, I would consider simplifying the first instruction by breaking it down into smaller pieces.
All compressed together as it is, it is some serious black magic and should be quite tough to read or maintain by anyone else.

But $("body").on("click", "#close", closeM); is very simple, and also much more contained because it applies to the only element with id #close in the HTML, instead of to all the images. So I would prefer to leave it separate.


Socket.io Node.js Object Oriented Code Structure Approach following application:
Users can join Rooms. A Gameroom inherits from Room and has a queue where Users can queue for a game. If enough people queue they get thrown into a game lobby. Lobby also inherits from Room. Rooms implement chat functionality which should be used in Gamerooms and Lobbies aswell.

Right now I am passing the io Object to the instantiations and use it to listen to events specific to their class features.

What improvements can I do? Why is this a bad or good design choice? What alternatives are there? What is there to consider?
Should I split  the classes "Room", "Gameroom" and "Lobby" into a more distinct Model & Controller?

UML Diagram

Server instantiates the Rooms.
Server.js

const io = socket.listen(
    http.createServer(app).listen(8080)
  );

console.log('Server listening..');

app.get('/', function (req, res) {
    res.sendFile(__dirname + '/test.html');
  });

var main = new Room('', io);
var a = new Gameroom('a', io);
var b = new Gameroom('b', io);

Room is the parent class, but can also be instantiated. It should be a root channel where everyone joins when they login.
Room.js

class Room {
  constructor(name, io) {
    this.name = name;
    this.users = [];
    this.namespace = io.of('/' + name);
    this.listenOnRoom();
  }

  listenOnRoom() {
    this.namespace.on('connection', (socket) => {
      ...

      socket.on('disconnect', (msg) => {
        ...
      });

      socket.on('chatMessage', (msg) => {
        ...
      });
    });
  }
}
module.exports = Room;

Gameroom adds the ability to queue for a game.
Gameroom.js

class Gameroom extends Room {
  constructor(name, io) {
    super(name, io);
    this.io = io;
    this.queue = [];
    this.lobbies = [];
    this.listenOnGameroom();
  }

  listenOnGameroom() {
    this.namespace.on('connection', (socket) => {

      socket.on('userJoinQueue', () => {
        ...
        this.lobbies.push(new Lobby(this.name ,lobbyId, participants, this.io));
      });

      socket.on('userLeaveQueue', () => {
        ...
      });

      socket.on('userLeaveLobby', () => {
        ...
      });
    });
  }
}

module.exports = Gameroom;

Lobby is instantiated by the Gameroom when there are enough players in the queue.
Lobby.js

class Lobby extends Room {
  constructor(gameroom, name, users, io) {
    super(gameroom + '/' + name, io);
    this.users = users;
    this.readyUsers = [];
    this.listenOnLobby();
  }

  listenOnLobby() {
    this.namespace.on('connection', (socket) => {

      socket.on('userReady', () => {
        ...
      });

      socket.on('userUnready', () => {
        ...
      });
    });
  }
}

module.exports = Lobby;

Just a few comments:

1:
The Room class can quickly become a place to throw in all sorts of functionality, even when it's not needed by all its children.
I would be careful to not put much there at all, and rather compose the different rooms with needed functionality.
For example, a GameRoom may be composed by a chat, game queue, lobbies, and even user management.
Using composition like this can make it much easier to build different types of rooms, without having to throw every bit of common code into Room. I would even consider removing Room altogether if possible.

2:
In your socket callbacks, I would only put in single function calls to methods in your class.
This way, if you want to test one of these functions, you don't need to go via the socket callbacks.
Also, when instantiating your classes, I would consider not running the listener immediately.
If you're testing this, you don't want the hassle of mocking up io if you don't need to.

But I think an even better way to do this would be to abstract away the socket stuff from these classes entirely.
This could be a bit too much though, depending on the complexity of the app.

3:
Consider replacing

this.lobbies.push(new Lobby(this.name ,lobbyId, participants, this.io));

with something like

this.lobbies.push(this.lobbyFactory.create(this.name, lobbyId, participants));

This is useful for testing, since you don't force the usage of one particular lobby.
It's also useful if you decide to have different types of lobbies later on.
Then you can just inject another type of lobbyFactory.


Recursive material name printing logic I have a function which goes through every name in an object. This object has an array parameter materials which holds other objects. Those objects have the same array parameter which hold more objects and so on.

My code lines 3-5 just repeat while adding .material and another variable. Also the code will only go into an object's name four layers down or else I have to keep repeating more code.

How can I cut down on this mess?

var itemList = function(x) {
    console.log(x.materials[0].name);
    for (var i = 1; i < x.materials.length; i++) {
    console.log(x.materials[i].name);
       if (x.materials[i].build !== "BasicFactory" && i !== 0) {
           for (var j = 1; j < x.materials[i].materials.length; j++) {
               console.log(x.materials[i].materials[j].name);
               if (x.materials[i].materials[j].build !== "BasicFactory" && j !== 0) {
                   for (var k = 1; k < x.materials[i].materials[j].materials.length; k++) {
                   console.log(x.materials[i].materials[j].materials[k].name);
                   if (x.materials[i].materials[j].materials[k].build !== "BasicFactory" && j !== 0) {
                       for (var l = 1; l < x.materials[i].materials[j].materials[k].materials.length; l++) {
                       console.log(x.materials[i].materials[j].materials[k].materials[l].name);
                    }
                }
              }
            }
        }
    }
}
};

You could call itemList recursively for each object in x.materials:

var itemList = function(x) {
    for (var i = 0; i < x.materials.length; i++) {
        console.log(x.materials[i].name);
        if (x.materials[i].build !== "BasicFactory" && i !==0) {
            itemList(x.materials[i]);
        }
    }
};


Vertically placing the words in a string Please let me know a better approach to solve below problem:

Problem: Vertically arrange the words in a string and print on console.

Sample Input:

Hello Jack

Output:

H J
e a
L c
L k
o

My solution:

public static void toVerticalWords(String str){

        //split the words by whitespace
        String[] strArr = str.split("\\s");
        int maxWordLen = 0;

        //get the longest word length
        for(String strTemp : strArr) {
            if(strTemp.length() > maxWordLen)
                maxWordLen = strTemp.length();
        }

        //make a matrix of the words with each character in an array block
        char[][] charArr = new char[strArr.length][maxWordLen];
        for(int i=0; i<strArr.length; i++) {
            int j=0;
            for(char ch : strArr[i].toCharArray()){
                charArr[i][j] = ch;
                j++;
            }
        }

        //print the vertical word pattern, or transpose of above matrix (2D array)
        for(int j=0; j<maxWordLen; j++) {
            for(int i=0; i<strArr.length; i++) {
                if (i!=0)
                    System.out.print(" ");
                System.out.print(charArr[i][j]);
            }

            System.out.println();
        }
    }

I assume you are only trying to work with ASCII characters, but I would like to point out a few Unicode-related failure modes, which can be exhibited by the following code:

String[] funkyStrings = {
    "foo bar",
    "foo ba\u0308r",
    "\ud835\udcbb\u2134\u2134 \ud835\udcb7\ud835\udcb6\ud835\udcc7"
};
for (String str : funkyStrings) {
    System.out.println(str);
    toVerticalWords(str);
}

The first string produces ordinary output:

foo bar
f b
o a
o r

The second string contains a grapheme cluster that is made up from multiple code points – the a and an accent ̈ which together create the glyph ä (← that's the precomposed form showing how it should look, as not all fonts handle combined characters correctly)

foo bär
f b
o a
o ̈
 r

The third string uses mathematical glyphs which have code points beyond U+FFFF. Because Java is slightly Unicode-retarded and can't deal with such high code points properly, its chars are actually UTF-16 code units (16 bit wide, far to small for what we are dealing with here). Therefore, higher code points have to be specified as a pair of surrogate halves. The encoding for U+1D4BB is d835 dcbb in UTF-16BE. This now produces the following output:

ℴℴ
? ?
? ?
ℴ ?
ℴ ?
 ?
 ?

Only the ℴ is displayed correctly, because it has a lower code point of U+2134. For the other characters, the surrogate halves are separated from each other, leading to invalid characters.

Not shown here: how fullwidth and halfwidth charaters can mess up your vertical layout.

The moral of the story: code points, UTF-16 code units, characters and glyphs are different things. Dealing with them correctly is excessively difficult, and I don't know about any suitable tools. If you are not prepared to deal correctly with any input you may encounter, then check that the input conforms to a subset you are comfortable with.

for (char c : str) {
    if (c > 0x7F) {
        throw new IllegalArgumentException("The input string may only contain ASCII code points");
    }
}


Xml serialize to object I have to read some data from xml file and then need to store the converted json in the database. The class DtoProcedureXml mapped with xml attributes.

public class ProcedureManager
{
    #region Declaration
    private DtoProcedureXml procedure;
    private ProcedureDataManager procedureDataManager;
    #endregion

    public ProcedureManager()
    {
        this.procedureDataManager = new ProcedureDataManager();
    }

    public bool SaveProcedureData(long procedureID)
    {
        bool isSuccess = false;
        try
        {
            SetProcedureXmlData();
            isSuccess = SaveProcedureSummary(procedureID);
        }
        catch (Exception ex)
        {
            CommonLogger.LogException("Failed to save procedure data");
        }
        return isSuccess;
    }

    private bool SaveProcedureSummary(long procedureID)
    {
        //saving the data to db
        return procedureDataManager.Save(new DtoProcedure { ProcedureID = procedureID, JsonData = Newtonsoft.Json.JsonConvert.SerializeObject(this.procedure), CreatedOn = DateTime.Now });
    }

    private void SetProcedureXmlData()
    {
        this.procedure = new DtoProcedureXml();
        XmlSerializer serializer = new XmlSerializer(typeof(DtoProcedureXml));
        using (XmlTextReader reader = new XmlTextReader(FileShareUrlBuilder.GetPath(Constants.Structures.PathType.XMLFILEPATH)))
        {
            object obj = serializer.Deserialize(reader);
            this.procedure = (DtoProcedureXml)obj;
            reader.Close();
        }
    }
}

I have some concerns with the function SetSReportXmlData() When I was debugging the XmlSerializer code takes few milliseconds, is it right to move that code in the using section?
Also please let me know if there is any efficient solutions than this logic.

I would suggest not to recreate the instance of XmlSerializer each time, because it creates an assembly in memory that is not reused / unloaded. To avoid that, just create the instance as static field of the class or use something like a XmlSerializerCache.

see also: XmlSerializer class may result in a memory leak and poor performance

The value of procedure could be returned by SetProcedureXmlData and passed to SaveProcedureSummary. That makes it more clear the the methods have to be called in that order.
The instance variable procedureDataManager could be readonly
You could drop the isSuccess variable if you return the result of SaveProcedureSummary or false in the catch block.
It would be more readable if the line in SaveProcedureSummary is splitted to 2 or 3 lines.
The using is OK, but ther is no need to close the reader because it will be closed when the reader is disposed by the using.


Javascript Queue Object I created a Queue object when answering this question and I was wondering if it could do with some improvement.

Here is the current code:

var Queue = (function () {

    Queue.prototype.autorun = true;
    Queue.prototype.running = false;
    Queue.prototype.queue = [];

    function Queue(autorun) {
        if (typeof autorun !== "undefined") {
            this.autorun = autorun;
        }
        this.queue = []; //initialize the queue
    };

    Queue.prototype.add = function (callback) {
        var _this = this;
        //add callback to the queue
        this.queue.push(function () {
            var finished = callback();
            if (typeof finished === "undefined" || finished) {
                //  if callback returns `false`, then you have to
                //  call `next` somewhere in the callback
                _this.dequeue();
            }
        });

        if (this.autorun && !this.running) {
            // if nothing is running, then start the engines!
            this.dequeue();
        }

        return this; // for chaining fun!
    };

    Queue.prototype.dequeue = function () {
        this.running = false;
        //get the first element off the queue
        var shift = this.queue.shift();
        if (shift) {
            this.running = true;
            shift();
        }
        return shift;
    };

    Queue.prototype.next = Queue.prototype.dequeue;

    return Queue;

})();

Is there anything that I can expand on or improve in this Object?
Should anything be changed?

Overall, the code is quite simple. That's a good thing already.

Some things can be improved though:

Always create the autorun property. It will help your compiler. See here for more details. Besides, it's simpler to read. Just go with this:

function Queue(autorun) {
    this.autorun = autorun;
}

On another note, adding the properties on the prototype is just as bad, read the link I already showed to understand why.

Maybe it's only me, but the first lines are disturbing because they're before the "class" declaration. So it looks like they belong to the external object, while they actually belong to the hoisted function.
You don't need typeof variable === "undefined". variable === undefined is enough because you know for sure the identifier exists.
var shift = this.queue.shift(); w-w-w-what's this variable name?! You probably mean var first .... Use descriptive variable names.
I'm not sure you want to return shift in dequeue. Don't you mean this? Chaining is fun!


Unit of measure class for mass in java I was looking at this answer on SO to handle units in java.  The concept was interesting to me, except I didn't like that it had to be per object that had mass, instead of encapsulating a Mass class on its own so the pattern need not be repeated.  This is what I came up with.

public class Main
{

  public static void main(String args[]){
    Mass x = Mass.from_oz(2.3);
    System.out.println("x in kg: " + x.get_kg());
    System.out.println("x in oz: " + x.get_oz());

    Mass y = Mass.from_lb(1.2);
    System.out.println("y in kg: " + y.get_kg());
    System.out.println("y in lb: " + y.get_lb());

    Mass z = Mass.add(x, y);
    System.out.println("x + y in kg: " + z.get_kg());

    z.add(Mass.from_kg(2.0));
    System.out.println("x + y + 2.0kg in kg: " + z.get_kg());

  }
}

public class Mass{
    private double mass;
    private static double LB_PER_KG = 2.20462;
    private static double OZ_PER_LB = 16;

    private static double lb_to_kg(double value){
        return value * (1/LB_PER_KG);
    }
    private static double kg_to_lb(double value){
        return value * (LB_PER_KG);
    }
    private static double kg_to_oz(double value){
        return value * (LB_PER_KG*OZ_PER_LB);
    }
    private static double oz_to_kg(double value){
        return value * (1/(LB_PER_KG*OZ_PER_LB));
    }

    public double get_kg(){
        return this.mass;
    }
    public double get_lb(){
        return kg_to_lb(this.mass);
    }
    public double get_oz(){
        return kg_to_oz(this.mass);
    }

    public void set_kg(double value){
        this.mass = value;
    }
    public void set_lb(double value){
        this.mass = lb_to_kg(value);
    }
    public void set_oz(double value){
        this.mass = oz_to_kg(value);
    }

    private Mass(double value){
        set_kg(value);
    }

    public static Mass from_kg(double value){
        return new Mass(value);
    }
    public static Mass from_lb(double value){
        return new Mass(lb_to_kg(value));
    }
    public static Mass from_oz(double value){
        return new Mass(oz_to_kg(value));
    }

    public Mass add(Mass rhs){
        this.mass += rhs.mass;
        return this;
    }
    public Mass sub(Mass rhs){
        this.mass -= rhs.mass;
        return this;
    }
    public Mass div(Mass rhs){
        this.mass *= rhs.mass;
        return this;
    }
    public Mass mul(Mass rhs){
        this.mass /= rhs.mass;
        return this;
    }

    public static Mass add(Mass lhs, Mass rhs){
        return new Mass(lhs.mass).add(rhs);
    }
    public static Mass sub(Mass lhs, Mass rhs){
        return new Mass(lhs.mass).sub(rhs);
    }
    public static Mass mul(Mass lhs, Mass rhs){
        return new Mass(lhs.mass).mul(rhs);
    }
    public static Mass div(Mass lhs, Mass rhs){
        return new Mass(lhs.mass).div(rhs);
    }

}

Looking for any criticism, in addition to answers to these questions:

am I doing class arithmetic in java right?  It doesn't have overloading of operators, so I'm not sure what the convention is for arithmetic.
I didn't use lower camel case for the unit part of functions and variables, despite lower camel case being the convention, I felt it was harder to see toKg or toKG especially when this theoretically could be used with different units where Nm and nm would be two different things.
is there a better way to manage the conversion functions, seem like I have to add two every time I add a new unit?  what about units that are power of 10 convertible like for kg vs g?  Is there a special pattern I could employ there (eg kg, g, mg, etc...)?

For just Mass, I would define some constants.

public final static Mass kg = new Mass(1);
public final static Mass lb = new Mass(1/2.2, kg);
public final static Mass oz = new Mass(1/16.0, lb);

And define your conversions with the “to” (and “from”) units passed in.

public double in(Mass unit) {
    return mass / unit.mass;
}

public static double convert(double quantity, Mass from, Mass to) {
    return quantity * from.mass / to.mass;
}

Of course, you’d need to remove the “set” members, and make the Mass a constant value class.

private final double mass;

public Mass(double kg) {
    mass =  kg;
}

public Mass(double quantity, Mass unit) {
    mass = quantity * unit.mass;
}

New units just mean defining new class constants.

Mass weight = new Mass( 100, Mass.kg );
double pounds = weight.in( Mass.lb );
double ounces = weight.in( Mass.oz );

As described in the comments, you could expand this to a complete unit calculator, if you tracked the power of each unit dimension in the quantity class.


Getting the shortest sequence to reach a number I want to get the shortest sequence to reach a number.

Rules:

A[0] is always 1
Either A[i] = A[i-1]*2 or A[i] = A[i-1]+1

For example, if my input number is 17, my output should be 6, because

A[0]=1, A[1]=2, A[2]=4, A[3]=8, A[4]=16, A[5]=17

The following code is not working properly for few inputs, such as 15.

public static int GetMinimumSequence(int n)
        {
            if (n == 1)
                return 1;

            int count = 1;
            int temp = 1;
            for (int i = 2; i <= n; i++)
            {
                temp = temp * 2;
                if (temp >= n)
                {
                    temp = temp/2;
                    temp = temp + 1;
                }
                count++;
                if(temp == n)
                    break;
            }
            return count;
        }

@CarySwoveland is on the right track.  The original code and several other solutions so far get the wrong answer.  For example, GetMinimumSequence(12) should not return 8, but rather 5, based on the optimal sequence 1, 2, 3, 6, 12.

One straightforward solution uses recursion:

public static int GetMinimumSequence(int n) {
    if (n <= 0) {
        throw new ArgumentOutOfRangeException();
    } else if (n == 1) {
        return 1;
    } else if ((n % 2) == 1) {
        return 1 + GetMinimumSequence(n - 1);
    } else {
        return 1 + Math.Min(GetMinimumSequence(n - 1),
                            GetMinimumSequence(n / 2));
    }
}

Add memoization to that for better performance with larger inputs.  (I'll leave that to you as an exercise.)  In technical terms, this solution uses dynamic programming, since the greedy algorithm leads you astray.


Minimum window substring The minimum window substring problem from leetcode.com asks:

  Given a string S and a string T, find the minimum window in S which will contain all the characters in T in complexity \$O(n)\$.

  For example,

  S = "ADOBECODEBANC"
  T = "ABC"

  Minimum window is "BANC".

I post two versions of code. The differences between two versions of code is, the first one will move the start index when there is a match, the 2nd version will not move the start index when there is a move.

Wondering if logics are both correct? If both have the correct logic, which one do you think has better performance? I have did basic testing and post my test cases below. Using Python 2.7.

from collections import defaultdict
import sys

def minWindow(source, pattern):
    targetCount = defaultdict(int)
    currentCount = defaultdict(int)
    for ch in pattern:
        targetCount[ch] += 1
    start = 0
    validCount = 0
    minSize = sys.maxint
    for index in range(len(source)):
        if source[index] not in targetCount:
            continue
        if currentCount[source[index]] < targetCount[source[index]]:
            validCount += 1
        currentCount[source[index]] += 1
        while validCount == len(targetCount):
            if source[start] not in targetCount:
                start += 1
            elif currentCount[source[start]] > targetCount[source[start]]:
                currentCount[source[start]] -= 1
                start += 1
            else:
                minSize = min(minSize, index-start+1)
                currentCount[source[start]] -= 1
                validCount -= 1
                start += 1
    return (minSize, start-1)

def minWindowv2(source, pattern):
    targetCount = defaultdict(int)
    currentCount = defaultdict(int)
    for ch in pattern:
        targetCount[ch] += 1
    start = 0
    validCount = 0
    minSize = sys.maxint
    for index in range(len(source)):
        if source[index] not in targetCount:
            continue
        if currentCount[source[index]] < targetCount[source[index]]:
            validCount += 1
        currentCount[source[index]] += 1
        while validCount == len(targetCount):
            if source[start] not in targetCount:
                start += 1
            elif currentCount[source[start]] > targetCount[source[start]]:
                currentCount[source[start]] -= 1
                start += 1
            else:
                minSize = min(minSize, index-start+1)
                break
    return (minSize, start)

if __name__ == "__main__":
    print minWindow("ADOBECODEBANC","ABC")
    print minWindowv2("ADOBECODEBANC", "ABC")

1. Review

I don't like comparative reviews, so I only looked in detail at the first version of your code. Possibly some of these comments apply to the other version too.

The code does not return the correct answer if there are repeated letters in pattern. For example, given S = 'ABCA' and T = 'AA', the minimum window should be the whole string. But:

>>> minWindow('ABCA', 'AA')
(1, 3)

To fix this, use len(pattern) instead of len(targetCount).
It would be better to raise an exception if no window is found rather than returning the meaningless result (4294967295, -1).
sys.maxint is not portable to Python 3. If you need a starting value that's bigger than any integer, use float('inf'). But it's possible to reorganize the code so that there's no need for a starting value — see the revised code below.
Instead of defaultdict(int), use Counter, which you can initialize directly from an iterable:

targetCount = Counter(pattern)

Instead of iterating over indexes into source, use enumerate:

for end, c in enumerate(source):
    if c not in targetCount:
        continue
    if currentCount[c] < targetCount[c]:
        validCount += 1
    currentCount[c] += 1
    # ... and so on ...

This avoids lots of lookups of source[index].
There's repetition of lines like start += 1 and                 currentCount[source[start]] -= 1. It would be easy to reorder the code so that each line appears only once.
Similarly, instead of maintaining and incrementing a start index, create an iterable that enumerates the source string:

window_start = enumerate(source)

and call next each time you need another character from it:

start, c = next(window_start)
if c not in targetCount:
    continue
# ... and so on ...

This avoids lots of lookups of source[start].
The inner loop is:

while validCount == len(targetCount):

but validCount does not change on every loop iteration (only on loop iterations where the start character causes one of the current counts to drop below its target). So some of these tests are wasted. Instead, you could loop using:

for start, c in window_start:

and then exit the loop only after reducing validCount:

    validCount -= 1
    if validCount < len(pattern):
        break

The test:

if source[index] not in targetCount:
    continue

is not necessary — the algorithm works just as well without it. And similarly for the corresponding test in the inner loop.

2. Revised code

In this implementation I've tried to emphasize the symmetry between the outer and the inner loop. Really what we have here is a pair of coroutines, each iterating over the input, and yielding to the other one at the appropriate point.

from collections import Counter

def min_window(source, pattern):
    """Return the shortest substring of source that contains all the
    characters in pattern (with the required multiplicity). Raise
    ValueError if there is no such substring.

    """
    def windows():
        pattern_length = len(pattern)
        target = Counter(pattern) # character -> required count
        current = Counter()       # character -> count in current window
        matched = 0               # matched characters in current window
        window_start, window_end = enumerate(source), enumerate(source)
        for end, c in window_end:
            current[c] += 1
            if current[c] <= target[c]:
                matched += 1
                if matched == pattern_length:
                    for start, c in window_start:
                        current[c] -= 1
                        if current[c] < target[c]:
                            matched -= 1
                            if matched < pattern_length:
                                yield end + 1 - start, start
                                break
    length, start = min(windows())
    return source[start: start + length]


Text 'guesser' / tumbler like animation I made a tumbler thingy...
Do you see possible bugs in the code or any improvements I could do?

var text = "0000 ASCII (Listeni/ˈæski/ ass-kee), abbreviated from American Standard Code for Information Interchange,[1] is a character-encoding scheme (the IANA prefers the name US-ASCII[2]). ASCII codes represent text in computers, communications equipment, and other devices that use text. Most modern character-encoding schemes are based on ASCII, though they support many additional characters. ASCII was the most common character encoding on the World Wide Web until December 2007, when it was surpassed by UTF-8, which includes ASCII as a subset.[3][4][5]";

// Returns a random integer between min and max.
function getRandomInt(min, max) {
  return Math.floor(Math.random() * (max - min + 1)) + min;
}

function tumbler(text) {
  var length = text.length;
  var tumbleText = "";

  function tumble() {
    var code, i, newTumbleText = "",
      rand;

    // Test if we're done.
    if (text === tumbleText) {
      return document.body.style.color = "white";
    }

    for (i = 0; i < length; i++) {
      // Compare the two chars.
      if (text[i] !== tumbleText[i]) {
        code = text.charCodeAt(i);
        rand = getRandomInt(code - 10 < 0 ? 0 : code - 10, code + 10);
        newTumbleText += String.fromCodePoint(rand);
      } else {
        newTumbleText += text[i];
      }
    }

    tumbleText = newTumbleText;
    document.body.textContent = tumbleText;
    window.setTimeout(tumble, 60);
  }

  tumble();
}

tumbler(text);
body {
  background: black;
  color: lime;
  font-family: Courier, monospace;
}

Pretty fun little bit of code. Very neatly written as well. Only thing I would suggest would be to use .charAt(x) instead of using array indexers for your strings when you're comparing characters. Not a big deal but will improve compatibility among browsers and is a bit easier to read IMHO.

// Compare the two chars.
if (text.charAt(i) !== tumbleText.charAt(i))

Good stackoverflow post covers it in more detail here.


Reducing @media queries In my webpage, I have the following @media queries:

/* Smartphones (iPhone 3GS/4 and others; portrait and landscape) */
@media only screen and (min-width: 320px) and (max-width: 480px){
    html {
        font-size: 60%;
    }
}

/* iPhone 5 (portrait and landscape) */
@media only screen and (min-width: 320px) and (max-width: 568px){
    html {
        font-size: 60%;
    }
}

/* iPhone 6 (portrait and landscape) */
@media only screen and (min-width: 375px) and (max-width: 667px){
    html {
        font-size: 60%;
    }
}

/* iPhone 6 Plus (portrait) */
@media only screen and (min-width: 414px) and (max-width: 736px) and (orientation : portrait){
    html {
        font-size: 60%;
    }
}

/* Other smartphones with smaller screen (portrait and landscape) */
@media only screen and (max-width : 319px){
    html {
        font-size: 50%;
    }
}

/* iPads 1/2 iPad Mini (portrait and landscape) */
@media only screen and (min-device-width : 768px) and (max-device-width : 1024px) and (-webkit-min-device-pixel-ratio : 1) {
    html {
        font-size: 90%;
    }
}

/* iPad 3/4 (portrait and landscape) */
@media only screen and (min-device-width : 768px) and (max-device-width : 1024px) and (-webkit-min-device-pixel-ratio : 2) {
    html {
        font-size: 62.5%;
    }
}

/* Google Nexus 10 (portrait) */
@media only screen and (min-device-width : 800px) and (max-device-width : 1280px) and (orientation : portrait) {
    html {
        font-size: 100%;
    }
}

/* Google Nexus 7 (portrait) */
@media only screen and (min-device-width : 604px) and (max-device-width : 966px) and (orientation : portrait) {
    html {
        font-size: 100%;
    }
}

/* Google Nexus 7 2 (portrait and landscape) */
@media only screen and (min-device-width : 600px) and (max-device-width : 960px) {
    html {
        font-size: 100%;
    }
}

/* Desktops and laptops */
@media only screen  and (min-width : 1224px) {
    html {
        font-size: 100%;
    }
}

/* Large screens */
@media only screen  and (min-width : 1824px) {
    html {
        font-size: 150%;
    }
}

/* Motorola Nexus 6 (portrait and portrait) */
@media only screen and (min-device-width : 1440px) and (max-device-width : 2560px) and (-webkit-min-device-pixel-ratio : 3.5) {
    html {
        font-size: 200%;
    }
}

This works very well, but I am concerned because this is a lot of code and not a smart code.

What is the smarter way of writing these @media queries so that I dont have to write it for every device but it supports every device appropriately?

I would recommend a more generic approach, for example, I use a lot the Twitter’s Bootstrap variant from Nicholas Cerminara.
While your CSS looks like supports every device appropriately in terms of maintainability it is burdensome. For example most of the iPhone rules can be group into one. The same for the iPad and Nexus tablets.

If you take into consideration a responsive and mobile first styles approach, you can use media queries to only accommodate the styles that doesn’t fix into the mobile environment.

For example, the font size default value could be 60%. So this style doesn’t need to be written in the mobile media queries section, just in the desktop and other sizes. Your styles could be reduced to:

font-size: 60%;
/* Smartphones (iPhone portrait and landscape) */
@media only screen and (min-width: 320px) and (max-width: 480px){
    html {
        /* font-size: 60% is the default now */
    }
}

/* Small Devices, Tablets */
@media only screen and (min-width : 768px) {
    html {
        font-size: 90%;
    }
}

/* Medium Devices, Desktops */
@media only screen and (min-width : 992px) {
    html {
        font-size: 120%;
    }
}

/* Large Devices, Wide Screens */
@media only screen and (min-width : 1200px) {
    html {
        font-size: 200%;
    }
}

And still covers, (if all others style and tags are properly architectured for responsive design) all previous requirements.


Reverse a linked list from position m to n, in-place and in one pass /**
 * Definition for singly-linked list.
 * struct ListNode {
 *     int val;
 *     ListNode *next;
 *     ListNode(int x) : val(x), next(NULL) {}
 * };
 */

class Solution {
public:
    ListNode *reverseBetween(ListNode *head, int m, int n)
     {
        ListNode *pre=NULL, *cur=head, *front=NULL, *rear=NULL,*follow=head->next;
        int subLoop=n-m;
        if(m==1 && subLoop==0)
        {
            return head;
        }

        if(m==1)
        {
            rear=head;
        }
        else
        {
            for(int i=0;i<m-1;++i)
            {
                if(i==m-2)
                {
                    front=cur;
                }
                cur=cur->next;
            }
            rear=cur;
        }
        while(cur!=NULL&&subLoop-->=0)
        {
            follow=cur->next;
            cur->next=pre;
            pre=cur;
            cur=follow;
        }
        if(m!=1)
        {
            front->next=pre;

        }
        else if(m==1&&pre!=NULL)
        {
            head= pre;
        }
        rear->next=cur;
        return head;

    }
};

This is hard to read and follow:

    ListNode *pre=NULL, *cur=head, *front=NULL, *rear=NULL,*follow=head->next;

One variable per line:

This is really hard to read:

while(cur!=NULL&&subLoop-->=0)

Not sure what the --> operator does :-(

Your code could really do with a lot more white space between operators to make it readable.

To be honest I can not follow the logic of this function. Especially since I have not idea what the input parameters are supposed to represent:

ListNode *reverseBetween(ListNode *head, int m, int n)

What are m and n for?


Fold large input lines into smaller ones I am working on a program that is meant to fold a large input line into smaller ones. Basic flow of the program would be as follows:

User calls the program with a long string as input
Program prints tokens generated in separate lines

I haven't seen handling file input yet, so keep that in mind. I haven't seen reading input from the command line either.

Here's the source code:

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>

int length(char input[]);

int main(int argc, char* argv[])
{

    if(argc < 3)
    {
        printf("Usage: %s input [n]\n", argv[0]);
        exit(1);
    }

    int COL_BOUND = atof(argv[2]);

    int startCol, currentCut, nextBound, numOfCuts;

    startCol = currentCut = nextBound = numOfCuts = 0;

    //while the next bound is still "inside" string
    while((nextBound = startCol + COL_BOUND - 1) <= length(argv[1]))
    {
        //find cut character
        for(int i = startCol; i <= nextBound; ++i)
        {
            if(isspace(argv[1][i]) && !isspace(argv[1][i-1]))
                currentCut = i - 1;
        }
        //there was no valid "cut" before whitespace or bound itself is valid
        if(!isspace(argv[1][nextBound]) || currentCut <= startCol)
            currentCut = nextBound;

        //print [startCol, cut]
        int printed = 0;
        for(int i = startCol; i <= currentCut; ++i)
        {
            if(isspace(argv[1][i]) && printed == 0)
                continue;
            ++printed;
            putchar(argv[1][i]);
        }

        printf("\n");

        //adjust start column for the next run
        startCol = startCol + COL_BOUND;
        ++numOfCuts;
    }

    //print remaining characters
    int printed = 0;
    ++numOfCuts;
    for(int i=startCol; i <= length(argv[1]); ++i)
    {
        if(isspace(argv[1][i]) && printed == 0)
            continue;
        ++printed;
        putchar(argv[1][i]);
    }

    printf("\nOriginal string split into %d parts (bound was %d)\n", numOfCuts, COL_BOUND);
    return 0;
}

int length(char input[])
{
    int i;
    for(i=0; input[i] != '\0'; ++i)
        ;
    return i;
}

The length() function can be replaced using strlen() (you'll need to include <string.h>).  Note that strlen() doesn't consider the terminating NUL to be part of the string - but that will help fix the current bug that causes this code to print that NUL.

Don't measure the length repeatedly - instead, save it to a variable early on, and refer to that variable.

Error messages should go to standard error stream, not standard output:

if (argc < 3) {
    fprintf(stderr, "Usage: %s input cols\n", argv[0]);
    return 1;
}

Don't use uppercase names for variables - we reserve those for macros; the "SHOUTING" of the name warns us that they need special care.

Also, don't use atof() for converting integers - use atoi(), or better, strtoul(), which can tell you whether it succeeded:

char *parse_end;
size_t col_bound = strtoul(argv[2], &parse_end, 10);
if (parse_end == argv[2] || *parse_end) {
    fprintf(stderr, "Usage: %s input cols\n", argv[0]);
    return 1;
}

Prefer one declaration per line, and initialise immediately:

size_t startCol = 0;
size_t currentCut = 0;
size_t nextBound = 0;
size_t numOfCuts = 0;

Be careful about off-by-one errors.  This inequality should be <, not <=:

while ((nextBound = startCol + col_bound - 1) < length) {

It's not the only one.

Test more inputs.  I tried a very simple case and got unexpected output:

./194156 'a b c d e f' 3

a b
c
d e
f
Original string split into 4 parts(bound was 3)

Why did it not split into 3 lines?


Binary tree pruning with BFS Below the text of the exercise:

  We are given the head node root of a binary tree, where additionally
  every node's value is either a 0 or a 1.

  Return the same tree where every subtree (of the given tree) not
  containing a 1 has been removed.

  (Recall that the subtree of a node X is X, plus every node that is a
  descendant of X.)

  Note:

  The binary tree will have at most 100 nodes.
  The value of each node will only be 0 or 1.

I have the following code and it does not look nice to me as too many if statements strolling around. How can I make this shorter and nice looking (more readable). Tree has nodes that contain 0 or 1 as value. I make the node null if it does not contain any node having 1 as value.

public TreeNode pruneTree(TreeNode root) {
    if (root == null || (root.left == null && root.right == null && root.val == 0)) return null;

    Queue<TreeNode> queue = new LinkedList<>();
    queue.offer(root);

    while(!queue.isEmpty()) {
        TreeNode node = queue.poll();

        if (node.left != null && !containsOne(node.left)) {
            node.left = null;
        }

        if (node.right != null && !containsOne(node.right)) {
            node.right = null;
        }

        if (node.left != null) {
            queue.offer(node.left);
        }

        if (node.right != null) {
            queue.offer(node.right);
        }
    }

    return root;
}

private boolean containsOne(TreeNode node) {
    if (node == null) return false;
    if (node.val == 1) return true;
    return containsOne(node.left) || containsOne(node.right);
}

Note: I checked binary tree pruning and the structure of class TreeNode is not modifiable as I expected:

public class TreeNode {
    int val;
    TreeNode left;
    TreeNode right;
    TreeNode(int x) { val = x; }
}

It's possible to shorten your code with constraints of the class, your method containsOne is the following:

private boolean containsOne(TreeNode node) {
    if (node == null) return false;
    if (node.val == 1) return true;
    return containsOne(node.left) || containsOne(node.right);
}

Because the third line will be executed when the condition node.val == 1 is false put directly this condition in the or expression in the third line:

private static boolean containsOne(TreeNode node) {
    if (node == null) return false;
    return node.val == 1 || containsOne(node.left) || containsOne(node.right);
}

About your method PruneTree you can shorten the following lines inside the method:

if (node.left != null && !containsOne(node.left)) {
    node.left = null;
}
if (node.right != null && !containsOne(node.right)) {
    node.right = null;
}
if (node.left != null) {
    queue.offer(node.left);
}
if (node.right != null) {
    queue.offer(node.right);
}

The code can be rewritten like below, two equal blocks and not so elegant to see but the original structure in the site cannot be modified, so I haven't thought about other alternatives:

if (node.left != null) {
    if (!containsOne(node.left)) { node.left = null; }
    else { queue.offer(node.left); }
}

if (node.right != null) {
    if (!containsOne(node.right)) { node.right = null; }
    else { queue.offer(node.right); }
}

I checked the code on the site passing all tests.


A "todo" app built to practice Object Oriented JavaScript I built a small "todo" app with an object oriented JavaScript approach.

I have spent this morning reading on a few ways to approach this, and I've come up with the following:

document.addEventListener("DOMContentLoaded", function () {
 var todo = Object.create(null);

 todo.newTodo = function (evt) {
  evt.preventDefault();
  var newTodo = document.getElementById("todo-item").value;
  var ul = document.getElementById("todo-view");
  var li = document.createElement("li");
  li.innerHTML = newTodo + 'x';

  //using tertnary operator to add items to top of list if other items already exist
  (ul.childElementCount == 0) ? ul.appendChild(li) : ul.insertBefore(li, ul.firstChild);

  document.getElementById('todo-item'). value = "";

  var span = li.getElementsByTagName("span");

  span[0].addEventListener("click", todo.deleteTodo, false);
 }

 var todoCache = [];

 todo.deleteTodo = function () {
  todoCache.push(this.parentNode);

  this.parentNode.parentNode.removeChild(this.parentNode);

  return todoCache;
 }

 todo.undoDelete = function () {
  var ul = document.getElementById("todo-view");

  if (todoCache.length > 0 ) {
   var lastTodo = todoCache.length - 1;

   ul.appendChild(todoCache[lastTodo]);

   todoCache.pop();
  }
 }

 var todoSub = document.getElementById("submit-item");

 todoSub.addEventListener("click", todo.newTodo, false);

 var undo = document.getElementById('undo');

 undo.addEventListener("click", todo.undoDelete, false);
});
 body, html {
  width: 100%;
  height: 100%;
  margin: 0;
  padding: 0;
 }

 body {
  background: #ccc;
  font-family: 'Open Sans Condensed', sans-serif;
  font-weight: 100;
 }

 .list-container {
  padding: 15px 10px;
  margin: 0 auto;
  max-width: 30em;
  text-align: center;
 }

 .input {
  width: 100%;
 }

 .input input {
  height: 50px;
  display: block;
  width: 100%;
  margin: 0 auto;
  box-sizing: border-box;
  transition: linear .2s;
 }

 input[type="text"] {
  padding: 10px 20px;
  margin: 10px auto;
  border: none;

 }

 input[type="submit"] {
  background: #7FFFD4;
  border: none;
  color: white;
  padding: 0;
  display: block;
  cursor: pointer;
  font-size: 15px;
  text-transform: uppercase;
 }

 input[type="submit"]:hover {
  background-color: #66ccaa;
 }

 h1 {
  text-transform: uppercase;
  font-size: 40px;
  color: #fff;
 }

 ul {
  margin: 0;
  padding: 0;
 }

 li {
  list-style-type: none;
  padding: 7px 0;
  width: 100%;
  transition: .2s linear;
  position: relative;
 }

 li:hover span {
  display: inline-block;
 }

 li span {
  position: absolute;
  right: 10px;
  color: red;
  margin-right: 5px;
  display: none;
  cursor: pointer;
  font-size: 20px;
 }

 a {
  color: white;
 }

 <meta charset="UTF-8">

<link href='https://fonts.googleapis.com/css?family=Open+Sans+Condensed:300' rel='stylesheet' type='text/css'>

TODO List

 UNDO

  <form>
   <input id="todo-item" type="text" placeholder="Enter your tasks here...">
   <input id="submit-item" type="submit" value="add">
  </form>

What do you suggest i do to improve this? Is there anything else you recommend to help further my learning efforts?

First, your markup. From first look, it looks like a loose set of elements. I cannot tell which parts are which immediately. One method you can use is BEM. It's primarily a CSS naming convention, but can easily be used to identify which elements belong to a group.

TODO List

  UNDO

something

  <form class="todo__form>
    <input class="todo__input" type="text" placeholder="Enter your tasks here...">
    <input class="todo__submit" type="submit" value="add">
  </form>

In this case, you have a todo block. Inside this todo block you have todo__element. If these elements have alternate states or styles from their default, you have todo__element--alternate. Simple naming convention.

This leads us to the next issue, CSS. First, the way you write CSS is very generic. You're styling using element selectors. This is fine if the app is isolated, but if you do this inside a bigger app, you risk affecting other elements not related to the todo list.

.todo{...}
.todo__header{...}
.todo__undo{...}
/* and so on */

In this case, in conjunction with the BEM naming convention, you can easily target elements with class names. They all have the same specificity since they're all classes, and you can easily identify which groups of styles belong together and which groups of elements they affect.

Before we head over to JS, I notice that you have UNDO. Instead of putting a blank inline script on href, I suggest you put # and use event.preventDefault() in the click handler to prevent it from navigating. This is so that all your JS live in JS, not mixed anywhere.

Now in JS, I notice that you don't keep track of state in the script. You submit a todo, directly append to the DOM, and that's it. JS knows nothing about the todo items afterwards and state is held in the DOM. This works in small scale, but this becomes tricky when the app scales, when you do more data manipulation like item counts, multiple undos and that sort of stuff.

Following MVC and similar variations, data should live in the data layer, or just some sort of storage like a variable. View is the UI, a visual representation of the data in the model. Controller accepts interaction, usually the place where handlers and logic are written. You don't need overly complex code, but it's nice to have them clearly separated.

(function(){

  var todoList   = document.querySelector('.todo__list');
  var todoSubmit = document.querySelector('.todo__submit');
  var todoInput  = document.querySelector('.todo__input');
  var todoUndo   = document.querySelector('.todo__undo');

  // Our "model", which is just some data storage
  var todos = [];

  // Our "controller", where all the logic goes
  function createTodo(item){
    // Return a todo object
  }

  function removeTodo(index){
    // Remove todo from todos at index
  }

  function addTodo(todo){
    // Add todo to todos
  }

  function renderTodos(){
    // For each item in todos, render an

    // Add or remove from the DOM accordingly.
  }

  todoSubmit.addEventListener('click', function(){
    // get input value
    // createTodo
    // addTodo
    // clear input
    // renderTodos
  });

  todoUndo.addEventListener('click', function(){
    // get last todo
    // removeTodo
    // renderTodos
  });

}());

In the really abstract sample above, you can see that todos is the source of truth, the "model". renderTodos simply renders that array. That plus your initial DOM constitutes your "view", the UI. The controller is the logic, from the handlers to the utility functions. The only time the JS has to know about the DOM is in renderTodos. Everything else is just JS, arrays and objects. You can easily determine the number of items by getting the array length and adjust your render function accordingly.

Now you may wonder, the delete button is dynamically generated with the todo, but how would you bind if renderTodo should only construct DOM? How would the controller bind to it? That's where event delegation comes in. Events "bubble" up the DOM. Click a descendant element, the ancestors also count as clicked. We can attach an event on a parent item to listen for future elements.

// Attach event handler to todoList (the ul)
todoList.addEventListener('click', function(event){
  // check if event.target is the delete button from an li
});


Projecteuler.net Problem 2 using collection pipeline Pattern I solve projecteuler.net Problem 2  deferent way

Generate number from 1 to range ex 100  and get the even number
Get Fibonacci numbers from list
Reduce array

I have one problem with a large set of numbers like 1000000 or 4000000.
can I ask how can I optimize the code? I am trying to solve the problem as a collection pipeline
swift code
var evenArray = ((0..<100).filter({$0 % 2 == 0}))

print(evenArray)

var evenFibArray = evenArray.filter {(isPerfectSquare(5 * $0 * $0  + 4) || isPerfectSquare(5 * $0 * $0  - 4))}

print(evenFibArray)

var result  = evenFibArray.reduce(into: 0){$0 += $1}

print(result)

func isPerfectSquare(_ x:Int ) -> Bool {
    let i = Int(sqrt(Double(x)))
    return  (i * i == x)
}

updated my code to use a deferent approach and apply collection pipeline
update my code

I use collection pipeline refactoring for more information about the
topics look
https://martinfowler.com/articles/refactoring-pipelines.html
I use stride function instead of filter to generate even Integer array

Sidenote: I don't know if I will save a couple of computing cycles—all I want to make my code more Swifty.
func isPerfectSquare(_ x:Int ) -> Bool {
    let i = Int(sqrt(Double(x)))
    return  (i * i == x)
}

var result  =  (Array(stride(from: 0`enter code here`, to: 100, by: 2)).filter{(isPerfectSquare(5 * $0 * $0  + 4) || isPerfectSquare(5 * $0 * $0  - 4))}).reduce(into: 0){$0 += $1}
print(result)